Message-ID: <20100514014404.GF8120@dastard>
Date:	Fri, 14 May 2010 11:44:04 +1000
From:	Dave Chinner <david@...morbit.com>
To:	"Aneesh Kumar K. V" <aneesh.kumar@...ux.vnet.ibm.com>
Cc:	hch@...radead.org, viro@...iv.linux.org.uk, adilger@....com,
	corbet@....net, serue@...ibm.com, neilb@...e.de,
	linux-fsdevel@...r.kernel.org, sfrench@...ibm.com,
	philippe.deniel@....FR, linux-kernel@...r.kernel.org
Subject: Re: [PATCH -V7 6/9] ext4: Add get_fsid callback

On Thu, May 13, 2010 at 12:02:52PM +0530, Aneesh Kumar K. V wrote:
> On Thu, 13 May 2010 13:11:33 +1000, Dave Chinner <david@...morbit.com> wrote:
> > On Wed, May 12, 2010 at 09:20:41PM +0530, Aneesh Kumar K.V wrote:
> > > Acked-by: Serge Hallyn <serue@...ibm.com>
> > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>
> > > ---
> > >  fs/ext4/super.c |   15 +++++++++++++++
> > >  1 files changed, 15 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > > index e14d22c..fc7d464 100644
> > > --- a/fs/ext4/super.c
> > > +++ b/fs/ext4/super.c
> > > @@ -1049,6 +1049,19 @@ static int bdev_try_to_free_page(struct super_block *sb, struct page *page,
> > >  	return try_to_free_buffers(page);
> > >  }
> > >  
> > > +static int ext4_get_fsid(struct super_block *sb, struct uuid *fsid)
> > > +{
> > > +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> > > +	struct ext4_super_block *es = sbi->s_es;
> > > +
> > > +	memcpy(fsid->uuid, es->s_uuid, sizeof(fsid->uuid));
> > > +	/*
> > > +	 * We may want to make sure we return error if the s_uuid is not
> > > +	 * exactly unique
> > > +	 */
> > > +	return 0;
> > > +}
> > > +
> > >  #ifdef CONFIG_QUOTA
> > >  #define QTYPE2NAME(t) ((t) == USRQUOTA ? "user" : "group")
> > >  #define QTYPE2MOPT(on, t) ((t) == USRQUOTA?((on)##USRJQUOTA):((on)##GRPJQUOTA))
> > > @@ -1109,6 +1122,7 @@ static const struct super_operations ext4_sops = {
> > >  	.quota_write	= ext4_quota_write,
> > >  #endif
> > >  	.bdev_try_to_free_page = bdev_try_to_free_page,
> > > +	.get_fsid	= ext4_get_fsid,
> > >  };
> > >  
> > >  static const struct super_operations ext4_nojournal_sops = {
> > > @@ -1128,6 +1142,7 @@ static const struct super_operations ext4_nojournal_sops = {
> > >  	.quota_write	= ext4_quota_write,
> > >  #endif
> > >  	.bdev_try_to_free_page = bdev_try_to_free_page,
> > > +	.get_fsid	= ext4_get_fsid,
> > >  };
> > 
> > This all looks pretty simple - can you add XFS support to this
> > interface (uuid is in XFS_M(sb)->m_sb.sb_uuid) so that it can be
> > tested to work on multiple filesystems.
> > 
> > FWIW, I didn't get patch 0 of this series, so I'll comment on
> > one line of it right here because it is definitely relevant:
> > 
> > > I am also looking at getting xfsprogs libhandle.so on top of these
> > > syscalls.
> > 
> > If you plan to modify libhandle to use these syscalls, then you need
> > to guarantee:
> > 
> > 	1. XFS support for the syscalls
> > 	2. the handle format, lifecycle and protections for XFS
> > 	   handles are *exactly* the same as the current XFS
> > 	   handles.  i.e. there's a fixed userspace API that
> > 	   cannot be broken.
> > 	3. you don't break any of the other XFS specific handle
> > 	   interfaces that aren't implemented by the new syscalls
> > 	3. You don't break and existing XFS utilites - dump/restore,
> > 	   and fsr come to mind immediately.
> > 	4. that you'll fix the xfstests that may break because of the
> > 	   change
> > 	5. that you'll write new tests for xfstests that validates
> > 	   that the libhandle API works correctly and that handle
> > 	   formats and lifecycles do not get accidentally changed in
> > 	   future.
> > 
> > That's a lot of work and, IMO, is completely pointless. All we'd get
> > out of it is more complexity, bloat, scope for regressions and a
> > biger test matrix, and we wouldn't have any new functionality to
> > speak of.
> 
> getting libhandle.so to work with the syscall is something that is
> suggested on the list. The goal is to see if syscall achieve everything
> that XFS ioctl does

Ok, I didn't know that, but the question still stands. The XFS ioctl
cannot go away any time soon (we basically have to support it
forever), so why should we be writing a new, redundant
kernel API for this functionality that is going not generally going
to be directly accessed by userspace developers?

APIs are hard to get right - moving and modifying kernel code to be
generic is easy in comparison, and also somethign we can easily fix
if we get it wrong the first time. Make a mistake with a syscall
API, and we are stuck with it forever.

Might I suggest a slightly different approach, then? That is,
separate the two parts of making the XFS handle code generic and
providing a new API?  We don't lose anything by separating them - we
don't introduce any new APIs that have to be supported in the first
step, nor does the functionality get delayed by API discussions.
However, we still gain immediate widespread support for handles through
the libraries *already shipping* in every major distro, and that
doesn't get held up by discussions around what the API should look
like.

Then we can work on getting a new API right - we're going to be
stuck with it forever, so it's probably better to work out how the
interface will be used outside libhandle. A new application using it
would be a great example - it's rare that an API created with only
one user is going to be a good API when more developers try to make
use of it for new applications.

There is precedence here - the FIFREEZE ioctl for freezing/thawing
filesystems from userspace іs the API that XFS has been using for
years (XFS_IOC_FREEZE) to provide the functionality. It got promoted
to the VFS when other filesystems needed userspace freezing
capabilities, but only after new syscalls were proposed first.  The
result of using the existing interface was that freeze/thaw for any
capable filesystem was immediately availble using xfs_freeze or
xfs_io - there was no lag to userspace support in distro's, no
problems having to detect and support multiple interfaces depending
on what the kernel supported, etc. Overall it made things much
simpler and easier to manage and test....

Your thoughts?

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists