export operations rewrite

PATCH In review Christoph Hellwig 3 participants 12 messages Mar 17, 2007 – May 14, 2007

Subsystems (3)

VFS NFS client NFS server

lore

Share & tools

lore

Christoph Hellwig

author

2007-03-17 01:09 UTC #0 lore raw

cover export operations rewrite

This patchset is a medium scale rewrite of the export operations interface. The goal is to make the interface less complex, and easier to understand from the filesystem side, aswell as preparing generic support for exporting of 64bit inode numbers.

This touches all nfs exporting filesystems, and I've done testing on all of the filesystems I have here locally (xfs, ext2, ext3, reiserfs, jfs)

Christoph Hellwig

author

2007-03-28 16:39 UTC #1 lore raw

Quoted 4 lines · Christoph Hellwig

This patchset is a medium scale rewrite of the export operations interface. The goal is to make the interface less complex, and easier to understand from the filesystem side, aswell as preparing generic support for exporting of 64bit inode numbers.

Any comments on this? So far I got two positive responses in private but no comment at all in public..

Neil Brown

2007-03-30 03:34 UTC #2 lore raw

In reply to Christoph Hellwig

On Saturday March 17, [email protected] wrote:

less that 2 weeks later....

Quoted 8 lines

This patchset is a medium scale rewrite of the export operations interface. The goal is to make the interface less complex, and easier to understand from the filesystem side, aswell as preparing generic support for exporting of 64bit inode numbers.
This touches all nfs exporting filesystems, and I've done testing on all of the filesystems I have here locally (xfs, ext2, ext3, reiserfs, jfs)

My only question involves motivation.

You say "less complex", but to me it just looks "different" - though being very familiar with the original, that might be a biased view. Can you say more about how it is less complex?
Maybe the extension to generic 64bit support will make that clear...

But then generic 64bit support should just be an independent set of helper functions that can be plugged in to the export_operations structure.

For what it does, the code looks fine, and I can see some definite improvements here and there, but I'm not clear on the over-all motivation.

Thanks,
NeilBrown

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Christoph Hellwig

author

2007-05-01 10:39 UTC #3 lore raw

Quoted 3 lines · Neil Brown

On Saturday March 17, [email protected] wrote:
less that 2 weeks later....

more than one month later.... :)

Quoted 7 lines

My only question involves motivation.
You say "less complex", but to me it just looks "different" - though being very familiar with the original, that might be a biased view. Can you say more about how it is less complex?
Maybe the extension to generic 64bit support will make that clear...

Quoted 7 lines

But then generic 64bit support should just be an independent set of helper functions that can be plugged in to the export_operations structure.
For what it does, the code looks fine, and I can see some definite improvements here and there, but I'm not clear on the over-all motivation.

When looking at suppor for 64bit inodes I stumbled over a few things I really dislike in the current code:

- I really hate the current function pointer indirection for find_exported_dentry. Using the same method table for communication in two different ways just feels very bad to me.
- passing the fid completely untyped to filesystem seem too error prone to me, and is rather contrary to how we do things elsewhere in the kernel.
- the calling conversion on the decode side where we first call ->decode_fh to split the filehandle into two blobs only to recurse back into exportfs and then recurse back into the filesystem seems rather odd. By having two methods to get the dentry and parent directly from the on the wire file handle this big callstack collapses to a very simple one.

So yes, we probably could do 64bit inode filehandles with the old scheme (and in fact xfs, ocfs2 and gfs2 already have support for it), but revamping the layering will make it quite a bit cleaner. I'll doug up those patches again, but on the decode side 64bit inode support is really trivial after these, it's just another case statement in the generic routines.

Neil Brown

2007-05-04 06:49 UTC #4 lore raw

Quoted 7 lines · On Tuesday May 1, [email protected] wrote:

Quoted 3 lines · Neil Brown
On Saturday March 17, [email protected] wrote:
less that 2 weeks later....
more than one month later.... :)

Thanks for your explanations.

Quoted 6 lines

- the calling conversion on the decode side where we first call ->decode_fh to split the filehandle into two blobs only to recurse back into exportfs and then recurse back into the filesystem seems rather odd. By having two methods to get the dentry and parent directly from the on the wire file handle this big callstack collapses to a very simple one.

This is the bit I was particularly missing. I see now how this aspect was awkward before, and how your changes make the flow clearer.

Getting rid of s_export_op->find_exported_dentry is something I'm very happy about. There was actually bug lurking there that I never got around to fixing. The code:
- /* Ok, we can export it */;
- if (!inode->i_sb->s_export_op->find_exported_dentry)
- inode->i_sb->s_export_op->find_exported_dentry =
- find_exported_dentry;

assumes that the address of find_exported_dentry will never change. But if you unload nfsd.ko, then re-load it. it could be different, yet a filesystem could still have to old value in it's s_export_op. That would be bad.
I'm glad it is gone.

Thanks,
NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Neil Brown

2007-05-06 23:52 UTC #5 lore raw

Quoted 19 lines · On Friday May 4, [email protected] wrote:

Quoted 6 lines · On Tuesday May 1, [email protected] wrote:
- the calling conversion on the decode side where we first call ->decode_fh to split the filehandle into two blobs only to recurse back into exportfs and then recurse back into the filesystem seems rather odd. By having two methods to get the dentry and parent directly from the on the wire file handle this big callstack collapses to a very simple one.
This is the bit I was particularly missing. I see now how this aspect was awkward before, and how your changes make the flow clearer.
Getting rid of s_export_op->find_exported_dentry is something I'm very happy about. There was actually bug lurking there that I never got around to fixing. The code:
(quoted diff) +0 -4
-	/* Ok, we can export it */;
-	if (!inode->i_sb->s_export_op->find_exported_dentry)
-		inode->i_sb->s_export_op->find_exported_dentry =
-			find_exported_dentry;

Quoted 5 lines

assumes that the address of find_exported_dentry will never change. But if you unload nfsd.ko, then re-load it. it could be different, yet a filesystem could still have to old value in it's s_export_op. That would be bad.
I'm glad it is gone.

...but now I remember why it was there...

I didn't want filesystems to depend on exportfs.ko. nfsd, depends on exportfs, but currently filesystems don't. They can be compiled into the kernel without including exportfs.

With your patches in place, I got a compile error with a config in which nfsd is a module (and so exportfs is) but ext2 and ext3 are compiled in.
The error was that generic_fh_to_dentry and generic_fh_to_parent were not defined.

Now those two functions together with exportfs_d_alloc are fairly small and could go directly in fs/something.c. They could almost be inlined in linux/exportfs.h.

Would you be able to respin that second patch series with one of those changes?

Thanks,
NeilBrown

Neil Brown

2007-05-07 00:06 UTC #6 lore raw

Quoted 3 lines · On Monday May 7, [email protected] wrote:

Would you be able to respin that second patch series with one of those changes?

Of course it is actually the first series of patches that introduces this problem. So maybe just a full respin??

Thanks,
NeilBrown

Christoph Hellwig

author

2007-05-07 07:51 UTC #7 lore raw

Quoted 7 lines · Neil Brown

Quoted 3 lines · On Monday May 7, [email protected] wrote:
Would you be able to respin that second patch series with one of those changes?
Of course it is actually the first series of patches that introduces this problem. So maybe just a full respin??

I'll take a look at how to solve this issues.

Christoph Hellwig

author

2007-05-13 20:41 UTC #8 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 7 lines · Neil Brown
Quoted 3 lines · On Monday May 7, [email protected] wrote:
Would you be able to respin that second patch series with one of those changes?
Of course it is actually the first series of patches that introduces this problem. So maybe just a full respin??
I'll take a look at how to solve this issues.

I'll take some more time until I can look at this. Can you send of the first patch series to Linus in the meantime?

Neil Brown

2007-05-14 00:13 UTC #9 lore raw

Quoted 7 lines · On Sunday May 13, [email protected] wrote:

Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Neil Brown
Quoted 3 lines · On Monday May 7, [email protected] wrote:
Would you be able to respin that second patch series with one of those changes?

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Of course it is actually the first series of patches that introduces this problem. So maybe just a full respin??
I'll take a look at how to solve this issues.
I'll take some more time until I can look at this. Can you send of the first patch series to Linus in the meantime?

Oh.. the problem is in the second series, isn't it. I had somehow convinced myself it was in the first.
And the merge window has now closed, but I'll submit it to -mm shortly.
Sorry 'bout that.

NeilBrown

David Woodhouse

2007-04-01 15:03 UTC #10 lore raw

In reply to Christoph Hellwig

Quoted 3 lines · Christoph Hellwig

This touches all nfs exporting filesystems, and I've done testing on all of the filesystems I have here locally (xfs, ext2, ext3, reiserfs, jfs)

modprobe mtdram ; mount -tjffs2 mtd0 /mnt

Signature · 11 lines

dwmw2


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Christoph Hellwig

author

2007-05-13 20:43 UTC #11 lore raw

Quoted 6 lines · David Woodhouse

Quoted 3 lines · Christoph Hellwig
This touches all nfs exporting filesystems, and I've done testing on all of the filesystems I have here locally (xfs, ext2, ext3, reiserfs, jfs)
modprobe mtdram ; mount -tjffs2 mtd0 /mnt

There's no nfs exporting support in mainline for jffs2. And yes, I didn't forget my promise to help you sorting out how to do it properly, but I'm a it too busy right now - I'll do it after I've fixed up the issues with this patch series.