Message-ID: <542411.1582194875@warthog.procyon.org.uk>
Date:   Thu, 20 Feb 2020 10:34:35 +0000
From:   David Howells <dhowells@...hat.com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     dhowells@...hat.com, viro@...iv.linux.org.uk, raven@...maw.net,
        mszeredi@...hat.com, christian@...uner.io,
        linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/19] vfs: syscall: Add fsinfo() to query filesystem information [ver #16]

Darrick J. Wong <darrick.wong@...cle.com> wrote:

> > +	p->f_blocks.hi	= 0;
> > +	p->f_blocks.lo	= buf.f_blocks;
> 
> Er... are there filesystems (besides that (xfs++)++ one) that require
> u128 counters?  I suspect that the Very Large Fields are for future
> expandability, but I also wonder about the whether it's worth the
> complexity of doing this, since the structures can always be
> version-revved later.

I'm making a relatively cheap allowance for future expansion.  Dave Chinner
has mentioned at one of the LSFs that 16EiB may be exceeded soon (though I
hate to think of fscking such a beastie).  I know that the YFS variant of AFS
supports 96-bit vnode numbers (which I translate to inode numbers).  What I'm
trying to avoid is the problem we have with stat/statfs where under some
circumstances we have to return an error (ERANGE?) because we can't represent
the number if someone asks for an older version of the struct.

Since the buffer is (meant to be) pre-cleared, the filesystem can just ignore
the high word if it's never going to set it.  In fact, fsinfo_generic_statfs
doesn't need to set them either.

> XFS inodes are u64 values...
> ...and the max symlink target length is 1k, not PAGE_SIZE...

Yeah, and AFS(YFS) has 96-bit inode numbers.  The filesystem's fsinfo table is
read first so that the filesystem can override this.

> ...so is the usage model here that XFS should call fsinfo_generic_limits
> to fill out the fsinfo_limits structure, modify the values in
> ctx->buffer as appropriate for XFS, and then return the structure size?

Actually, I should export some these so that you can do that.  I'll export
fsinfo_generic_{timestamp_info,supports,limits} for now.

> > +#define FSINFO_ATTR_VOLUME_ID		0x05	/* Volume ID (string) */
> > +#define FSINFO_ATTR_VOLUME_UUID		0x06	/* Volume UUID (LE uuid) */
> > +#define FSINFO_ATTR_VOLUME_NAME		0x07	/* Volume name (string) */
> 
> I think I've muttered about the distinction between volume id and
> volume name before, but I'm still wondering how confusing that will be
> for users?  Let me check my assumptions, though:
> 
> Volume ID is whatever's in super_block.s_id, which (at least for xfs and
> ext4) is the device name (e.g. "sda1").  I guess that's useful for
> correlating a thing you can call fsinfo() on against strings that were
> logged in dmesg.
>
> Volume name I think is the fs label (e.g. "home"), which I think will
> have to be implemented separately by each filesystem, and that's why
> there's no generic vfs implementation.

Yes.  For AFS, for example, this would be the name of the volume (which may be
changed), whereas the volume ID is the number in the protocol that actually
refers to the volume (which cannot be changed).

And, as you say, for blockdev mounts, the ID is the device name and the volume
name is filesystem specific.

> The 7 -> 0 -> 1 sequence here confused me until I figured out that
> QUERY_TYPE is the mask for QUERY_{PATH,FD}.

Changed to FSINFO_FLAGS_QUERY_MASK.

> > +struct fsinfo_limits {
> > +...
> > +	__u32	__reserved[1];
> 
> I wonder if these structures ought to reserve more space than a single u32...

No need.  Part of the way the interface is designed is that the version number
for a particular VSTRUCT-type attribute is also the length.  So a newer
version is also longer.  All the old fields must be retained and filled in.
New fields are tagged on the end.

If userspace asks for an older version than is supported, it gets a truncated
return.  If it asks for a newer version, the extra fields it is expecting are
all set to 0.  Either way, the length (and thus the version) the kernel
supports is returned - not the length copied.

The __reserved fields are there because they represent padding (the struct is
going to be aligned/padded according to __u64 in this case).  Ideally, I'd
mark the structs __packed, but this messes with the alignment and may make the
compiler do funny tricks to get out any field larger than a byte.

I've renamed them to __padding.

> > +struct fsinfo_supports {
> > +	__u64	stx_attributes;		/* What statx::stx_attributes are supported */
> > +	__u32	stx_mask;		/* What statx::stx_mask bits are supported */
> > +	__u32	ioc_flags;		/* What FS_IOC_* flags are supported */
> 
> "IOC"?  That just means 'ioctl'.  Is this field supposed to return the
> supported FS_IOC_GETFLAGS flags, or the supported FS_IOC_FSGETXATTR
> flags?

FS_IOC_[GS]ETFLAGS is what I meant.

> I suspect it would also be a big help to be able to tell userspace which
> of the flags can be set, and which can be cleared.

How about:

	__u32	fs_ioc_getflags;	/* What FS_IOC_GETFLAGS may return */
	__u32	fs_ioc_setflags_set;	/* What FS_IOC_SETFLAGS may set */
	__u32	fs_ioc_setflags_clear;	/* What FS_IOC_SETFLAGS may clear */

> > +struct fsinfo_timestamp_one {
> > +	__s64	minimum;	/* Minimum timestamp value in seconds */
> > +	__u64	maximum;	/* Maximum timestamp value in seconds */
> 
> Given that time64_t is s64, why is the maximum here u64?

Well, I assume it extremely unlikely that the maximum can be before 1970, so
there doesn't seem any need to allow the maximum to be negative.  Furthermore,
it would be feasible that you could encounter a filesystem with a u64
filesystem that doesn't support dates before 1970.

On the other hand, if Linux is still going when __s64 seconds from 1970 wraps,
it will be impressive, but I'm not sure we'll be around to see it...  Someone
will have to cast a resurrection spell on Arnd to fix that one.

However, since signed/unsigned comparisons may have issues, I can turn it into
an __s64 also if that is preferred.

David

Powered by blists - more mailing lists