From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Aneesh Kumar K. V" Subject: Re: [PATCH -V7 6/9] ext4: Add get_fsid callback Date: Sat, 15 May 2010 11:39:26 +0530 Message-ID: <871vddelwp.fsf@linux.vnet.ibm.com> References: <1273679444-14903-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <1273679444-14903-7-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <20100513031133.GF13617@dastard> <87sk5w1fcb.fsf@linux.vnet.ibm.com> <20100514014404.GF8120@dastard> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: hch@infradead.org, viro@zeniv.linux.org.uk, adilger@sun.com, corbet@lwn.net, serue@us.ibm.com, neilb@suse.de, linux-fsdevel@vger.kernel.org, sfrench@us.ibm.com, philippe.deniel@CEA.FR, linux-kernel@vger.kernel.org To: Dave Chinner Return-path: Received: from e23smtp04.au.ibm.com ([202.81.31.146]:38144 "EHLO e23smtp04.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751328Ab0EOGJk convert rfc822-to-8bit (ORCPT ); Sat, 15 May 2010 02:09:40 -0400 In-Reply-To: <20100514014404.GF8120@dastard> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, 14 May 2010 11:44:04 +1000, Dave Chinner = wrote: > 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 wrote: > > > On Wed, May 12, 2010 at 09:20:41PM +0530, Aneesh Kumar K.V wrote: > > > > Acked-by: Serge Hallyn > > > > Signed-off-by: Aneesh Kumar K.V > > > > --- > > > > fs/ext4/super.c | 15 +++++++++++++++ > > > > 1 files changed, 15 insertions(+), 0 deletions(-) > > > >=20 > > > > 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); > > > > } > > > > =20 > > > > +static int ext4_get_fsid(struct super_block *sb, struct uuid *= fsid) > > > > +{ > > > > + struct ext4_sb_info *sbi =3D EXT4_SB(sb); > > > > + struct ext4_super_block *es =3D 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) =3D=3D USRQUOTA ? "user" : "group") > > > > #define QTYPE2MOPT(on, t) ((t) =3D=3D USRQUOTA?((on)##USRJQUOT= A):((on)##GRPJQUOTA)) > > > > @@ -1109,6 +1122,7 @@ static const struct super_operations ext4= _sops =3D { > > > > .quota_write =3D ext4_quota_write, > > > > #endif > > > > .bdev_try_to_free_page =3D bdev_try_to_free_page, > > > > + .get_fsid =3D ext4_get_fsid, > > > > }; > > > > =20 > > > > static const struct super_operations ext4_nojournal_sops =3D { > > > > @@ -1128,6 +1142,7 @@ static const struct super_operations ext4= _nojournal_sops =3D { > > > > .quota_write =3D ext4_quota_write, > > > > #endif > > > > .bdev_try_to_free_page =3D bdev_try_to_free_page, > > > > + .get_fsid =3D ext4_get_fsid, > > > > }; > > >=20 > > > 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. > > >=20 > > > 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: > > >=20 > > > > I am also looking at getting xfsprogs libhandle.so on top of th= ese > > > > syscalls. > > >=20 > > > If you plan to modify libhandle to use these syscalls, then you n= eed > > > to guarantee: > > >=20 > > > 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. > > >=20 > > > 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. > >=20 > > getting libhandle.so to work with the syscall is something that is > > suggested on the list. The goal is to see if syscall achieve everyt= hing > > that XFS ioctl does >=20 > 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? >=20 > 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. >=20 > 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 throu= gh > the libraries *already shipping* in every major distro, and that > doesn't get held up by discussions around what the API should look > like. >=20 > 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. >=20 > There is precedence here - the FIFREEZE ioctl for freezing/thawing > filesystems from userspace =D1=96s the API that XFS has been using fo= r > 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.... >=20 > Your thoughts? >=20 Howabout continuing with syscall patchset trying to see if we can get i= t merged in the next merge window. If it appears that a merge in the next merge window is difficult, I can definitely try the ioctl approach you outlined above -aneesh -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html