On Mon, Jun 23, 2025 at 3:18 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Mon 23-06-25 15:05:45, Amir Goldstein wrote:
> > 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...
>
> No strong preference if we then test for equality with FILEID_PIDFS and
> FILEID_DRM and not like fh_type & FILEID_PIDFS.
>
> > 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.
>
> I'm getting lost in these names a bit :) It's hard to see a difference for
> me without a concrete examples of where one should be used compared to the
> other...
In any case, I don't feel strongly about it.
You can leave it as is or use
FILEID_PIDFS = 0x100,
or
FILEID_PIDFS = 0x180,
we can always change it later if we want to.
Thanks,
Amir.
