Re: UBIFS updates for 2.6.27

Re: UBIFS updates for 2.6.27

[Artem Bityutskiy]

Artem Bityutskiy wrote:
> Hi,
> 
> here is the second bunch of UBIFS updates we would like to send to
> Linus for 2.6.27 inclusion. The updates contain the following:

Oh, meant to send this to linux-fsdevel@vger.kernel.org, not to
linux-fsdevel-owner@vger.kernel.org. Apologies.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH] UBIFS: fill f_fsid

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

UBIFS stores 16-bit UUID in the superblock, and it is a good
idea to return part of it in 'f_fsid' filed of kstatfs structure.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/super.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 1207bd5..0dee404 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -386,6 +386,7 @@ static int ubifs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_files = 0;
 	buf->f_ffree = 0;
 	buf->f_namelen = UBIFS_MAX_NLEN;
+	memcpy(&buf->f_fsid, c->uuid, sizeof(__kernel_fsid_t));
 
 	return 0;
 }
-- 
1.5.4.1

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Sun, 2008-08-31 at 17:52 +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> UBIFS stores 16-bit UUID in the superblock, and it is a good
> idea to return part of it in 'f_fsid' filed of kstatfs structure.

I think you mean a 16-byte UUID, not 16-bit?

> +	memcpy(&buf->f_fsid, c->uuid, sizeof(__kernel_fsid_t));

For btrfs I xor the first 64 bits with the second 64 bits, and put
_that_ into f_fsid. You're just putting the first 64 bits in and
ignoring the second 64 bits. Neither is really _better_ than the other;
you just alter the circumstances in which you get collisions. But I
suppose we might as well be consistent about how we do it?

(Actually I XOR in the root object ID for the subvol too, but you don't
need to worry about that).

Alternatively, there's space in the struct statfs to export a couple
more uint32_ts, and thus the _whole_ of the uuid. Perhaps we should do
that?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] UBIFS: fill f_fsid

[Artem Bityutskiy]

On Mon, 2008-09-01 at 10:43 +0100, David Woodhouse wrote:
> > UBIFS stores 16-bit UUID in the superblock, and it is a good
> > idea to return part of it in 'f_fsid' filed of kstatfs structure.
> 
> I think you mean a 16-byte UUID, not 16-bit?

Oh, right.

> 
> > +	memcpy(&buf->f_fsid, c->uuid, sizeof(__kernel_fsid_t));
> 
> For btrfs I xor the first 64 bits with the second 64 bits, and put
> _that_ into f_fsid. You're just putting the first 64 bits in and
> ignoring the second 64 bits. Neither is really _better_ than the other;
> you just alter the circumstances in which you get collisions. But I
> suppose we might as well be consistent about how we do it?

Well, xor-ing should not make the random UUID more random, but for
consistency purposes sure, I may use xor as well.

> Alternatively, there's space in the struct statfs to export a couple
> more uint32_ts, and thus the _whole_ of the uuid. Perhaps we should do
> that?

You mean f_spare? I do not think any application would use it so I doubt
it is good idea to use it.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

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

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Mon, 2008-09-01 at 14:16 +0300, Artem Bityutskiy wrote:
> On Mon, 2008-09-01 at 10:43 +0100, David Woodhouse wrote:
> > > UBIFS stores 16-bit UUID in the superblock, and it is a good
> > > idea to return part of it in 'f_fsid' filed of kstatfs structure.
> > 
> > I think you mean a 16-byte UUID, not 16-bit?
> 
> Oh, right.
> 
> > 
> > > +	memcpy(&buf->f_fsid, c->uuid, sizeof(__kernel_fsid_t));
> > 
> > For btrfs I xor the first 64 bits with the second 64 bits, and put
> > _that_ into f_fsid. You're just putting the first 64 bits in and
> > ignoring the second 64 bits. Neither is really _better_ than the other;
> > you just alter the circumstances in which you get collisions. But I
> > suppose we might as well be consistent about how we do it?
> 
> Well, xor-ing should not make the random UUID more random, but for
> consistency purposes sure, I may use xor as well.
> 
> > Alternatively, there's space in the struct statfs to export a couple
> > more uint32_ts, and thus the _whole_ of the uuid. Perhaps we should do
> > that?
> 
> You mean f_spare? I do not think any application would use it so I doubt
> it is good idea to use it.

Well, it wouldn't be called f_spare any more, and applications would
know that it contains the rest of the UUID if it's non-zero.

I'd fix nfsd to fix it immediately (like as I just fixed nfsd to use the
first 64 bits of f_fsid when it's non-zero).

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] UBIFS: fill f_fsid

[Artem Bityutskiy]

On Mon, 2008-09-01 at 12:28 +0100, David Woodhouse wrote:
> > > Alternatively, there's space in the struct statfs to export a couple
> > > more uint32_ts, and thus the _whole_ of the uuid. Perhaps we should do
> > > that?
> > 
> > You mean f_spare? I do not think any application would use it so I doubt
> > it is good idea to use it.
> 
> Well, it wouldn't be called f_spare any more, and applications would
> know that it contains the rest of the UUID if it's non-zero.

May be, but there are only 5 bytes left. But why would you need more
UUID bytes?

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

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

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Mon, 2008-09-01 at 14:43 +0300, Artem Bityutskiy wrote:
> May be, but there are only 5 bytes left. 

There are 5 uint32_ts left. And then there would be 3.

> But why would you need more UUID bytes?

People are mostly using 16-byte UUIDs. We only have space to expose 8
bytes. It just seems to make sense to expose all 16.

But there's no overriding reason to do it, really.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] UBIFS: fill f_fsid

[Artem Bityutskiy]

David Woodhouse wrote:
> On Mon, 2008-09-01 at 14:43 +0300, Artem Bityutskiy wrote:
>> May be, but there are only 5 bytes left. 
> 
> There are 5 uint32_ts left. And then there would be 3.
O, right.

> But there's no overriding reason to do it, really.
Yeah, I would not go for it unless there is a specific problem
which should be solved with this.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
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

Re: [PATCH] UBIFS: fill f_fsid

[Christoph Hellwig]

On Mon, Sep 01, 2008 at 10:43:07AM +0100, David Woodhouse wrote:
> On Sun, 2008-08-31 at 17:52 +0300, Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > 
> > UBIFS stores 16-bit UUID in the superblock, and it is a good
> > idea to return part of it in 'f_fsid' filed of kstatfs structure.
> 
> I think you mean a 16-byte UUID, not 16-bit?
> 
> > +	memcpy(&buf->f_fsid, c->uuid, sizeof(__kernel_fsid_t));
> 
> For btrfs I xor the first 64 bits with the second 64 bits, and put
> _that_ into f_fsid. You're just putting the first 64 bits in and
> ignoring the second 64 bits. Neither is really _better_ than the other;
> you just alter the circumstances in which you get collisions. But I
> suppose we might as well be consistent about how we do it?

XFS just puts in the st_dev.  And I can't realy find any useful
defintion of what it's supposed to b anyway..

Re: [PATCH] UBIFS: fill f_fsid

[Artem Bityutskiy]

On Mon, 2008-09-01 at 11:01 -0400, Christoph Hellwig wrote:
> > For btrfs I xor the first 64 bits with the second 64 bits, and put
> > _that_ into f_fsid. You're just putting the first 64 bits in and
> > ignoring the second 64 bits. Neither is really _better_ than the other;
> > you just alter the circumstances in which you get collisions. But I
> > suppose we might as well be consistent about how we do it?
> 
> XFS just puts in the st_dev.  And I can't realy find any useful
> defintion of what it's supposed to b anyway..

For me this means that we should rather do what XFS does for
consistency then.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

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

Re: [PATCH] UBIFS: fill f_fsid

[Andreas Dilger]

On Sep 02, 2008  10:03 +0300, Artem Bityutskiy wrote:
> On Mon, 2008-09-01 at 11:01 -0400, Christoph Hellwig wrote:
> > > For btrfs I xor the first 64 bits with the second 64 bits, and put
> > > _that_ into f_fsid. You're just putting the first 64 bits in and
> > > ignoring the second 64 bits. Neither is really _better_ than the other;
> > > you just alter the circumstances in which you get collisions. But I
> > > suppose we might as well be consistent about how we do it?
> > 
> > XFS just puts in the st_dev.  And I can't realy find any useful
> > defintion of what it's supposed to b anyway..
> 
> For me this means that we should rather do what XFS does for
> consistency then.

The fsid is supposed to be a persistent, unique identifier for the
filesystem, used by NFS in file handles.  Using st_dev is unsafe,
because that may change from one server boot to the next, because
of device probing order, driver changes, etc.  Also, not all filesystems
HAVE a valid st_dev in the first place, which is the whole reason
for this thread.

I think a ->get_fsid() export method would be preferable.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Tue, 2008-09-02 at 11:09 -0600, Andreas Dilger wrote:
> The fsid is supposed to be a persistent, unique identifier for the
> filesystem, used by NFS in file handles.  Using st_dev is unsafe,
> because that may change from one server boot to the next, because
> of device probing order, driver changes, etc.  Also, not all
> filesystems
> HAVE a valid st_dev in the first place, which is the whole reason
> for this thread.

Yes, putting st_dev in f_fsid probably isn't a good thing to do.

> I think a ->get_fsid() export method would be preferable.

I implemented that, but it doesn't really work. The fsid->export mapping
happens in userspace, so mountd needs access to the fsid. So the mount
would work fine and return a FH with the appropriate fsid, and then
mountd would have no knowledge of how to handle that FH, and mount(8) on
the client would eventually time out and fail.

It worked if I prepopulated the nfsd.fh cache in expkey_parse() so we
didn't end up asking userspace to resolve that FH for us -- but that was
just a quick hack. It wouldn't have worked after a server reboot, for
example -- we'd have asked userspace again.

We'd need to export that fsid to userspace somehow. I did briefly think
about adding something like ',uuid=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx' to
each line in /proc/mounts, as if it was a file system option -- but I
don't like that much.

When looking for other options, I realised that we already have the
f_fsid field in struct statfs, and we might as well just use that. That
does seem to be what it was _designed_ for, after all.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] UBIFS: fill f_fsid

[Andreas Dilger]

On Sep 02, 2008  18:29 +0100, David Woodhouse wrote:
> Yes, putting st_dev in f_fsid probably isn't a good thing to do.
> 
> I realised that we already have the
> f_fsid field in struct statfs, and we might as well just use that. That
> does seem to be what it was _designed_ for, after all.

My bad - I had looked back in email archives about this issue and got
the export operation solution stuck in my head...

Yes, using f_fsid is the right way to do this, assuming NFSD plays along.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Tue, 2008-09-02 at 13:13 -0600, Andreas Dilger wrote:
> My bad - I had looked back in email archives about this issue and got
> the export operation solution stuck in my head...
> 
> Yes, using f_fsid is the right way to do this, assuming NFSD plays
> along.

It does now, if it can't find a UUID by other means.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] UBIFS: fill f_fsid

[Christoph Hellwig]

On Tue, Sep 02, 2008 at 11:09:54AM -0600, Andreas Dilger wrote:
> The fsid is supposed to be a persistent, unique identifier for the
> filesystem, used by NFS in file handles.  Using st_dev is unsafe,
> because that may change from one server boot to the next, because
> of device probing order, driver changes, etc.  Also, not all filesystems
> HAVE a valid st_dev in the first place, which is the whole reason
> for this thread.
> 
> I think a ->get_fsid() export method would be preferable.

Umm, different things.  f_fsid in stat(v)fs is just a cookie exported to
userspac that has never really been documented.

We also called the filesystem part of the NFS filehandle in a few
places, and for those it's correct that it should be stable.  Currently
the fsid is either created from the dev_t in kernelspace or from
uuids extracted through libuuid in userspace.

I can't see anything in the message that started this thread that
mentions NFS, btw.

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Tue, 2008-09-02 at 17:02 -0400, Christoph Hellwig wrote:
> On Tue, Sep 02, 2008 at 11:09:54AM -0600, Andreas Dilger wrote:
> > The fsid is supposed to be a persistent, unique identifier for the
> > filesystem, used by NFS in file handles.  Using st_dev is unsafe,
> > because that may change from one server boot to the next, because
> > of device probing order, driver changes, etc.  Also, not all filesystems
> > HAVE a valid st_dev in the first place, which is the whole reason
> > for this thread.
> > 
> > I think a ->get_fsid() export method would be preferable.
> 
> Umm, different things.  f_fsid in stat(v)fs is just a cookie exported to
> userspac that has never really been documented.
> 
> We also called the filesystem part of the NFS filehandle in a few
> places, and for those it's correct that it should be stable.  Currently
> the fsid is either created from the dev_t in kernelspace or from
> uuids extracted through libuuid in userspace.

Or from the f_fsid returned in statfs(), as of a week or so ago.

> I can't see anything in the message that started this thread that
> mentions NFS, btw.

That was Artem's motivation for filling in f_fsid in the first place.
Does anything else even _care_ about f_fsid? 

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] UBIFS: fill f_fsid

[Christoph Hellwig]

On Tue, Sep 02, 2008 at 10:26:09PM +0100, David Woodhouse wrote:
> > We also called the filesystem part of the NFS filehandle in a few
> > places, and for those it's correct that it should be stable.  Currently
> > the fsid is either created from the dev_t in kernelspace or from
> > uuids extracted through libuuid in userspace.
> 
> Or from the f_fsid returned in statfs(), as of a week or so ago.

I think that's a rather bad idea.  As mentioned before f_fsid is
basically random.

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Tue, 2008-09-02 at 17:48 -0400, Christoph Hellwig wrote:
> On Tue, Sep 02, 2008 at 10:26:09PM +0100, David Woodhouse wrote:
> > > We also called the filesystem part of the NFS filehandle in a few
> > > places, and for those it's correct that it should be stable.  Currently
> > > the fsid is either created from the dev_t in kernelspace or from
> > > uuids extracted through libuuid in userspace.
> > 
> > Or from the f_fsid returned in statfs(), as of a week or so ago.
> 
> I think that's a rather bad idea.  As mentioned before f_fsid is
> basically random.

It's not that bad. Of the file systems which actually fill it in...

For NTFS it's the volume serial number.

For ext[234] it's the UUID xor-folded into 64 bits.
For btrfs the same, but with the root object ID xor'd in too.

For bfs and xfs it's the block device -- which isn't ideal, as Andreas
points out, but is what we'd fall back to anyway.

BFS seems to have a s_volume field which perhaps we could use, but we
should check for it being all spaces and leave f_fsid as zero in that
case. Presumably XFS could be using sb_uuid?

For efs, it'll be either EFS_MAGIC or EFS_NEWMAGIC, which is probably a
bad thing if you want to export multiple EFS file systems. I'll send a
patch to fix that (by leaving it zero) shortly.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org                              Intel Corporation



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Tue, 2008-09-02 at 17:02 -0400, Christoph Hellwig wrote:
> Umm, different things.  f_fsid in stat(v)fs is just a cookie exported to
> userspac that has never really been documented.

Our man page for statfs(2) says...

 The f_fsid field

   Solaris,  Irix  and  POSIX have a system call statvfs(2) that returns a
   struct statvfs (defined in <sys/statvfs.h>) containing an unsigned long
   f_fsid.   Linux,  SunOS, HP-UX, 4.4BSD have a system call statfs() that
   returns a struct statfs (defined in <sys/vfs.h>)  containing  a  fsid_t
   f_fsid,  where  fsid_t  is defined as struct { int val[2]; }.  The same
   holds for FreeBSD, except that it uses the include file  <sys/mount.h>.

   The  general  idea  is that f_fsid contains some random stuff such that
   the pair (f_fsid,ino) uniquely determines a file.   Some  OSes  use  (a
   variation on) the device number, or the device number combined with the
   filesystem type.  Several OSes restrict giving out the f_fsid field  to
   the  superuser  only (and zero it for unprivileged users), because this
   field is used in the filehandle of the  filesystem  when  NFS-exported,
   and giving it out is a security concern.

   Under some OSes the fsid can be used as second parameter to the sysfs()
   system call.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] UBIFS: fill f_fsid

[Artem Bityutskiy]

On Tue, 2008-09-02 at 22:26 +0100, David Woodhouse wrote:
> That was Artem's motivation for filling in f_fsid in the first place.

Yes, right. I am planning NFS support and would like to have UBIFS
filling f_fsid.

> Does anything else even _care_ about f_fsid? 

I do not know anything but NFS.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

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

Re: [PATCH] UBIFS: fill f_fsid

[Artem Bityutskiy]

On Mon, 2008-09-01 at 10:43 +0100, David Woodhouse wrote:
> For btrfs I xor the first 64 bits with the second 64 bits, and put
> _that_ into f_fsid. You're just putting the first 64 bits in and
> ignoring the second 64 bits. Neither is really _better_ than the other;
> you just alter the circumstances in which you get collisions. But I
> suppose we might as well be consistent about how we do it?
Like this?

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Wed, 3 Sep 2008 14:16:42 +0300
Subject: [PATCH] UBIFS: amend f_fsid

David Woodhouse suggested to be consistent with other FSes
and xor the beginning and the end of the UUID.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/super.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index ab9b5cb..83fe0a5 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -370,6 +370,7 @@ static int ubifs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	struct ubifs_info *c = dentry->d_sb->s_fs_info;
 	unsigned long long free;
+	__le32 *uuid = (__le32 *)c->uuid;
 
 	free = ubifs_get_free_space(c);
 	dbg_gen("free space %lld bytes (%lld blocks)",
@@ -386,8 +387,8 @@ static int ubifs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_files = 0;
 	buf->f_ffree = 0;
 	buf->f_namelen = UBIFS_MAX_NLEN;
-	memcpy(&buf->f_fsid, c->uuid, sizeof(__kernel_fsid_t));
-
+	buf->f_fsid.val[0] = le32_to_cpu(uuid[0]) ^ le32_to_cpu(uuid[2]);
+	buf->f_fsid.val[1] = le32_to_cpu(uuid[1]) ^ le32_to_cpu(uuid[3]);
 	return 0;
 }
 
-- 
1.5.4.1

Re: [PATCH] UBIFS: fill f_fsid

[David Woodhouse]

On Wed, 2008-09-03 at 12:44 +0300, Artem Bityutskiy wrote:
> On Mon, 2008-09-01 at 10:43 +0100, David Woodhouse wrote:
> > For btrfs I xor the first 64 bits with the second 64 bits, and put
> > _that_ into f_fsid. You're just putting the first 64 bits in and
> > ignoring the second 64 bits. Neither is really _better_ than the
> other;
> > you just alter the circumstances in which you get collisions. But I
> > suppose we might as well be consistent about how we do it?
>
> Like this?

Yeah, that's what ext[234] and btrfs do.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation