On Mon, Jun 23, 2025 at 2:41 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Mon 23-06-25 14:22:26, Amir Goldstein wrote:
> > On Mon, Jun 23, 2025 at 1:58 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Jun 23, 2025 at 01:55:38PM +0200, Jan Kara wrote:
> > > > On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> > > > > Introduce new pidfs file handle values.
> > > > >
> > > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> > > > > ---
> > > > > include/linux/exportfs.h | 11 +++++++++++
> > > > > 1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > > > index 25c4a5afbd44..45b38a29643f 100644
> > > > > --- a/include/linux/exportfs.h
> > > > > +++ b/include/linux/exportfs.h
> > > > > @@ -99,6 +99,11 @@ enum fid_type {
> > > > > */
> > > > > FILEID_FAT_WITH_PARENT = 0x72,
> > > > >
> > > > > + /*
> > > > > + * 64 bit inode number.
> > > > > + */
> > > > > + FILEID_INO64 = 0x80,
> > > > > +
> > > > > /*
> > > > > * 64 bit inode number, 32 bit generation number.
> > > > > */
> > > > > @@ -131,6 +136,12 @@ enum fid_type {
> > > > > * Filesystems must not use 0xff file ID.
> > > > > */
> > > > > FILEID_INVALID = 0xff,
> > > > > +
> > > > > + /* Internal kernel fid types */
> > > > > +
> > > > > + /* pidfs fid types */
> > > > > + FILEID_PIDFS_FSTYPE = 0x100,
> > > > > + FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
> > > >
> > > > What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
> > > > Why not just allocate one value for FILEID_PIDFS and be done with it? Do
> > > > you expect some future extensions for pidfs?
> > >
> > > I wouldn't rule it out, yes. This was also one of Amir's suggestions.
> >
> > The idea was to parcel the autonomous fid type to fstype (pidfs)
> > which determines which is the fs to decode the autonomous fid
> > and a per-fs sub-type like we have today.
> >
> > Maybe it is a bit over design, but I don't think this is really limiting us
> > going forward, because those constants are not part of the uapi.
>
> OK, I agree these file handles do not survive reboot anyway so we are free
> to redefine the encoding in the future. So it is not a big deal (but it
> also wouldn't be a big deal to start simple and add some subtyping in the
> future when there's actual usecase). But in the current patch set we have
> one flag FILEID_IS_AUTONOMOUS which does provide this subtyping and then
> this FILEID_PIDFS_FSTYPE which doesn't seem to be about subtyping but about
> pidfs expecting some future extensions and wanting to recognize all its
> file handle types more easily (without having to enumerate all types like
> other filesystems)? My concern is that fh_type space isn't that big and if
> every filesystem started to reserve flag-like bits in it, we'd soon run out
> of it. So I don't think this is a great precedens although in this
> particular case I agree it can be modified in the future if we decide so...
>
Yes, I agree.
For the sake of argument let's assume we have two types to begin with
pidfs and drm and then would you want to define them as:
/* Internal kernel fid types */
FILEID_PIDFS = 0x100,
FILEID_DRM = 0x200,
Or
FILEID_PIDFS = 0x100,
FILEID_DRM = 0x101,
I think the former is easy to start with and we have plenty of time to
make reparceling if we get to dousens and file id type...
Regarding the lower bits, I think it would be wise to reserve
FILEID_PIDFS_FSTYPE = 0x100,
FILEID_PIDFS_ROOT = FILEID_PIDFS_FSTYPE | FILEID_ROOT /* also 0x100 */
This is why I suggested using non zero lower bits and then why
not use the actual format descriptor for the lower bits as it was intended.
Thanks,
Amir.
