Re: [PATCH] btrfs: Add new flag to rescan quota after subvolume creation

[Marcos Paulo de Souza <mpdesouza@suse.de>]

On Tue, 2021-05-25 at 18:20 +0200, David Sterba wrote:
> On Fri, May 21, 2021 at 11:38:11AM -0300, Marcos Paulo de Souza
> wrote:
> > Adding a new subvolume/snapshot makes the qgroup data inconsistent,
> > so
> > add a new flag to inform the subvolume ioctl to do a quota rescan
> > right
> > after the subvolume/snapshot creation.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> > 
> >  This is an attempt to help snapper to create snapshots with
> > 'timeline'
> >  cleanup-policy to keep the qgroup data consistent after a new
> > snapshot is
> >  created.
> 
> I'm not sure adding something like rescan into subvolume creation. It
> can be started separately as a workaround. The problem I see is that
> there are two big things happening and both can fail, but there's
> only
> one return value and the user can tell which one failed.

Agreed. Indeed, it's much easier to issue another ioctl to do a quota
rescan.

> 
> > +	if (vol_args->flags & BTRFS_SUBVOL_QGROUP_RESCAN) {
> > +		if (!capable(CAP_SYS_ADMIN)) {
> > +			ret = -EPERM;
> 
> Eg. here, did the subvolume creation fail due to EPERM, or the
> rescan?

Agreed.

> 
> > +			goto free_args;
> > +		}
> > +
> > +		quota_rescan = true;
> > +	}
> > +
> >  	if (vol_args->flags & BTRFS_SUBVOL_RDONLY)
> >  		readonly = true;
> >  	if (vol_args->flags & BTRFS_SUBVOL_QGROUP_INHERIT) {
> > @@ -1843,6 +1869,22 @@ static noinline int
> > btrfs_ioctl_snap_create_v2(struct file *file,
> >  					subvol, readonly, inherit);
> >  	if (ret)
> >  		goto free_inherit;
> > +
> > +	if (quota_rescan) {
> > +		ret = do_quota_rescan(file);
> > +		/*
> > +		 * EINVAL is returned if quota is not enabled. There is
> > already
> > +		 * a warning issued if quota rescan is started when
> > quota is not
> > +		 * enabled, so skip a warning here if it is the case.
> > +		 */
> > +		if (ret < 0 && ret != -EINVAL)
> > +			btrfs_warn(btrfs_sb(file_inode(file)->i_sb),
> > +		"Couldn't execute quota rescan after snapshot creation:
> > %d",
> > +					ret);
> > +		else
> > +			ret = 0;
> 
> That's another special case and the system error message is required
> to
> interpret the error code but we've seen in the past that this is not
> a
> good usability pattern.

Agreed.

> 
> > +#define BTRFS_SUBVOL_QGROUP_RESCAN	(1ULL << 5)
> > +
> >  #define BTRFS_VOL_ARG_V2_FLAGS_SUPPORTED		\
> >  			(BTRFS_SUBVOL_RDONLY |		\
> >  			BTRFS_SUBVOL_QGROUP_INHERIT |	\
> >  			BTRFS_DEVICE_SPEC_BY_ID |	\
> > -			BTRFS_SUBVOL_SPEC_BY_ID)
> > +			BTRFS_SUBVOL_SPEC_BY_ID |	\
> > +			BTRFS_SUBVOL_QGROUP_RESCAN)
> 
> The idea of bits is to extend mode of operation of the ioctls, not to
> proxy other functionality. What I'd see as reasonable would be
> something
> like conditionally marking the quotas inconsistent by setting a bit
> after snapshot creation and when quotas are enabled. But the snapshot
> creation should do only snapshot creation.

It's much easier to change snapper in order to execute a quota rescan
when the quota data is inconsistent (due to a new snapshot being
created for example).