[PATCH 0/18] export operations rewrite

[PATCH 0/18] export operations rewrite

[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.

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)

Re: [NFS] [PATCH 0/18] export operations rewrite

[Christoph Hellwig]

On Sat, Mar 17, 2007 at 01:09:46AM +0000, Christoph Hellwig wrote:
> 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..

Re: [PATCH 0/18] export operations rewrite

[Neil Brown]

On Saturday March 17, hch@infradead.org wrote:

less that 2 weeks later....

> 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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH 0/18] export operations rewrite

[David Woodhouse]

On Sat, 2007-03-17 at 01:09 +0000, Christoph Hellwig wrote:
> 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

:)

-- 
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS] [PATCH 0/18] export operations rewrite

[Christoph Hellwig]

On Fri, Mar 30, 2007 at 01:34:53PM +1000, Neil Brown wrote:
> On Saturday March 17, hch@infradead.org wrote:
> 
> less that 2 weeks later....

more than one month later.... :)

> 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.

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.

Re: [PATCH 0/18] export operations rewrite

[Neil Brown]

On Tuesday May 1, hch@infradead.org wrote:
> On Fri, Mar 30, 2007 at 01:34:53PM +1000, Neil Brown wrote:
> > On Saturday March 17, hch@infradead.org wrote:
> > 
> > less that 2 weeks later....
> 
> more than one month later.... :)
> 

Thanks for your explanations.

>  - 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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS] [PATCH 0/18] export operations rewrite

[Neil Brown]

On Friday May 4, neilb@suse.de wrote:
> On Tuesday May 1, hch@infradead.org 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:
> -	/* 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.

...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

Re: [PATCH 0/18] export operations rewrite

[Neil Brown]

On Monday May 7, neilb@suse.de 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

-------------------------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS] [PATCH 0/18] export operations rewrite

[Christoph Hellwig]

On Mon, May 07, 2007 at 10:06:10AM +1000, Neil Brown wrote:
> On Monday May 7, neilb@suse.de 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.

Re: [NFS] [PATCH 0/18] export operations rewrite

[Christoph Hellwig]

On Mon, May 07, 2007 at 08:51:00AM +0100, Christoph Hellwig wrote:
> On Mon, May 07, 2007 at 10:06:10AM +1000, Neil Brown wrote:
> > On Monday May 7, neilb@suse.de 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?

Re: [NFS] [PATCH 0/18] export operations rewrite

[Christoph Hellwig]

On Sun, Apr 01, 2007 at 04:03:25PM +0100, David Woodhouse wrote:
> On Sat, 2007-03-17 at 01:09 +0000, Christoph Hellwig wrote:
> > 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.

Re: [NFS] [PATCH 0/18] export operations rewrite

[Neil Brown]

On Sunday May 13, hch@infradead.org wrote:
> On Mon, May 07, 2007 at 08:51:00AM +0100, Christoph Hellwig wrote:
> > On Mon, May 07, 2007 at 10:06:10AM +1000, Neil Brown wrote:
> > > On Monday May 7, neilb@suse.de 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?

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