Date:   Wed, 3 Feb 2021 16:21:51 +0100
From:   Miklos Szeredi <mszeredi@...hat.com>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Andreas Dilger <adilger@...ger.ca>,
        Andreas Gruenbacher <agruenba@...hat.com>,
        Christoph Hellwig <hch@....de>,
        "Darrick J . Wong" <djwong@...nel.org>,
        Dave Kleikamp <shaggy@...nel.org>,
        David Sterba <dsterba@...e.com>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Joel Becker <jlbec@...lplan.org>,
        Matthew Garrett <matthew.garrett@...ula.com>,
        Mike Marshall <hubcap@...ibond.com>,
        Richard Weinberger <richard@....at>,
        Ryusuke Konishi <konishi.ryusuke@...il.com>,
        "Theodore Ts'o" <tytso@....edu>, Tyler Hicks <code@...icks.com>
Subject: Re: [PATCH 01/18] vfs: add miscattr ops

On Wed, Feb 3, 2021 at 4:05 PM Jan Kara <jack@...e.cz> wrote:

[...]
> > +/**
> > + * miscattr_fill_xflags - initialize miscattr with xflags
> > + * @ma:              miscattr pointer
> > + * @xflags:  FS_XFLAG_* flags
> > + *
> > + * Set ->fsx_xflags, ->xattr_valid and ->flags (translated xflags).  All
> > + * other fields are zeroed.
> > + */
> > +void miscattr_fill_xflags(struct miscattr *ma, u32 xflags)
>
> Maybe call this miscattr_fill_from_xflags() and the next function
> miscattr_fill_from_flags()? At least to me it would be clearer when I want
> to use which function just by looking at the name...

Yes, more clarity for the cost of a longer name.  I'm not sure...

[...]
> > +/**
> > + * vfs_miscattr_get - retrieve miscellaneous inode attributes
> > + * @dentry:  the object to retrieve from
> > + * @ma:              miscattr pointer
> > + *
> > + * Call i_op->miscattr_get() callback, if exists.
> > + *
> > + * Returns 0 on success, or a negative error on failure.
> > + */
> > +int vfs_miscattr_get(struct dentry *dentry, struct miscattr *ma)
> > +{
> > +     struct inode *inode = d_inode(dentry);
> > +
> > +     if (d_is_special(dentry))
> > +             return -ENOTTY;
> > +
> > +     if (!inode->i_op->miscattr_get)
> > +             return -ENOIOCTLCMD;
> > +
> > +     memset(ma, 0, sizeof(*ma));
>
> So here we clear whole 'ma' but callers already set e.g. xattr_valid field
> and cleared the 'ma' as well which just looks silly...

Well spotted.   Fixed.

[...]
> > +/**
> > + * vfs_miscattr_set - change miscellaneous inode attributes
> > + * @dentry:  the object to change
> > + * @ma:              miscattr pointer
> > + *
> > + * After verifying permissions, call i_op->miscattr_set() callback, if
> > + * exists.
> > + *
> > + * Verifying attributes involves retrieving current attributes with
> > + * i_op->miscattr_get(), this also allows initilaizing attributes that have
> > + * not been set by the caller to current values.  Inode lock is held
> > + * thoughout to prevent racing with another instance.
> > + *
> > + * Returns 0 on success, or a negative error on failure.
> > + */
> > +int vfs_miscattr_set(struct dentry *dentry, struct miscattr *ma)
> > +{
> > +     struct inode *inode = d_inode(dentry);
> > +     struct miscattr old_ma = {};
> > +     int err;
> > +
> > +     if (d_is_special(dentry))
> > +             return -ENOTTY;
> > +
> > +     if (!inode->i_op->miscattr_set)
> > +             return -ENOIOCTLCMD;
> > +
> > +     if (!inode_owner_or_capable(inode))
> > +             return -EPERM;
> > +
> > +     inode_lock(inode);
> > +     err = vfs_miscattr_get(dentry, &old_ma);
> > +     if (!err) {
> > +             /* initialize missing bits from old_ma */
> > +             if (ma->flags_valid) {
> > +                     ma->fsx_xflags |= old_ma.fsx_xflags & ~FS_XFLAG_COMMON;
> > +                     ma->fsx_extsize = old_ma.fsx_extsize;
> > +                     ma->fsx_nextents = old_ma.fsx_nextents;
> > +                     ma->fsx_projid = old_ma.fsx_projid;
> > +                     ma->fsx_cowextsize = old_ma.fsx_cowextsize;
> > +             } else {
> > +                     ma->flags |= old_ma.flags & ~FS_COMMON_FL;
> > +             }
> > +             err = miscattr_set_prepare(inode, &old_ma, ma);
> > +             if (!err)
> > +                     err = inode->i_op->miscattr_set(dentry, ma);
>
> So I somewhat wonder here - not all filesystems support all the xflags or
> other extended attributes. Currently these would be just silently ignored
> AFAICT. Which seems a bit dangerous to me - most notably because it makes
> future extensions of these filesystems difficult. So how are we going to go
> about this? Is every filesystem supposed to check what it supports and
> refuse other stuff (but currently e.g. your ext2 conversion patch doesn't do
> that AFAICT)? Shouldn't we make things easier for filesystems to provide a
> bitmask of changing fields (instead of flags / xflags bools) so that they
> can refuse unsupported stuff with a single mask check?

Ah, ext2 one is missing miscattr_has_xattr() check and doesn't use the
miscattr_fill_flags() helper.  It was one of the earlier fs I
converted, and the API wasn't so refined then.  Fixed.

Will review all conversions too for this type of omission.

Creating a  mask instead of bool makes sense, I'll look into this.

> To make things more complex, ext2/4 has traditionally silently cleared
> unknown flags for setflags but not for setxflags. Unlike e.g. XFS which
> refuses unknown flags.

Right. Not sure if this can be fixed.  Documenting rules and
exceptions should be a first step.

Thanks,
Miklos

Powered by blists - more mailing lists