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

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

[Marcos Paulo de Souza]

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.

 Please let me know if you know a better place to add this functionality.

 fs/btrfs/ioctl.c           | 58 +++++++++++++++++++++++++++++---------
 include/uapi/linux/btrfs.h |  8 ++++--
 2 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 5dc2fd843ae3..64b4aa744486 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1704,6 +1704,22 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 	return ret;
 }
 
+static long do_quota_rescan(struct file *file)
+{
+	struct inode *inode = file_inode(file);
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+	int ret;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		return ret;
+
+	ret = btrfs_qgroup_rescan(fs_info);
+
+	mnt_drop_write_file(file);
+	return ret;
+}
+
 static noinline int __btrfs_ioctl_snap_create(struct file *file,
 				const char *name, unsigned long fd, int subvol,
 				bool readonly,
@@ -1793,6 +1809,7 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 	struct btrfs_ioctl_vol_args_v2 *vol_args;
 	int ret;
 	bool readonly = false;
+	bool quota_rescan = false;
 	struct btrfs_qgroup_inherit *inherit = NULL;
 
 	if (!S_ISDIR(file_inode(file)->i_mode))
@@ -1808,6 +1825,15 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 		goto free_args;
 	}
 
+	if (vol_args->flags & BTRFS_SUBVOL_QGROUP_RESCAN) {
+		if (!capable(CAP_SYS_ADMIN)) {
+			ret = -EPERM;
+			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;
+	}
+
 free_inherit:
 	kfree(inherit);
 free_args:
@@ -4277,35 +4319,25 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
 
 static long btrfs_ioctl_quota_rescan(struct file *file, void __user *arg)
 {
-	struct inode *inode = file_inode(file);
-	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_ioctl_quota_rescan_args *qsa;
 	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	ret = mnt_want_write_file(file);
-	if (ret)
-		return ret;
-
 	qsa = memdup_user(arg, sizeof(*qsa));
-	if (IS_ERR(qsa)) {
-		ret = PTR_ERR(qsa);
-		goto drop_write;
-	}
+	if (IS_ERR(qsa))
+		return PTR_ERR(qsa);
 
 	if (qsa->flags) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	ret = btrfs_qgroup_rescan(fs_info);
+	ret = do_quota_rescan(file);
 
 out:
 	kfree(qsa);
-drop_write:
-	mnt_drop_write_file(file);
 	return ret;
 }
 
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index 5df73001aad4..8779aa4b3aad 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -47,11 +47,14 @@ struct btrfs_ioctl_vol_args {
 
 #define BTRFS_SUBVOL_SPEC_BY_ID	(1ULL << 4)
 
+#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)
 
 #define BTRFS_FSID_SIZE 16
 #define BTRFS_UUID_SIZE 16
@@ -119,7 +122,8 @@ struct btrfs_ioctl_qgroup_limit_args {
 /* Supported flags for BTRFS_IOC_SNAP_CREATE_V2 and BTRFS_IOC_SUBVOL_CREATE_V2 */
 #define BTRFS_SUBVOL_CREATE_ARGS_MASK					\
 	 (BTRFS_SUBVOL_RDONLY |						\
-	 BTRFS_SUBVOL_QGROUP_INHERIT)
+	 BTRFS_SUBVOL_QGROUP_INHERIT |					\
+	 BTRFS_SUBVOL_QGROUP_RESCAN)
 
 /* Supported flags for BTRFS_IOC_SNAP_DESTROY_V2 */
 #define BTRFS_SUBVOL_DELETE_ARGS_MASK					\
-- 
2.26.2

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

[David Sterba]

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.

> +	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?

> +			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.

> +#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.

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

[Marcos Paulo de Souza]

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).