| Message ID | 5aff5ceb6555f8026f414c4de9341c698837820b.1688597211.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: simple quotas | expand |
On Wed, Jul 05, 2023 at 04:20:44PM -0700, Boris Burkov wrote: > Pull creating the qgroup earlier in the snapshot. This allows simple > quotas qgroups to see all the metadata writes related to the snapshot > being created and to be born with the root node accounted. > > Signed-off-by: Boris Burkov <boris@bur.io> > --- > fs/btrfs/qgroup.c | 3 +++ > fs/btrfs/transaction.c | 5 +++++ > 2 files changed, 8 insertions(+) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index 75afd8212bc0..8419270f7417 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -1670,6 +1670,9 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid) > struct btrfs_qgroup *qgroup; > int ret = 0; > > + if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED) > + return 0; > + > mutex_lock(&fs_info->qgroup_ioctl_lock); > if (!fs_info->quota_root) { > ret = -ENOTCONN; > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index f644c7c04d53..2bb5a64f6d84 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1716,6 +1716,11 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > } > btrfs_release_path(path); > > + ret = btrfs_create_qgroup(trans, objectid); > + if (ret) { > + btrfs_abort_transaction(trans, ret); > + goto fail; > + } Newline please. How is this ok with normal qgroups? We weren't creating a qgroup at snapshot creation time at all it seems, so I don't understand how this is ok for qgroups. Thanks, Josef
On Thu, Jul 13, 2023 at 10:26:00AM -0400, Josef Bacik wrote: > On Wed, Jul 05, 2023 at 04:20:44PM -0700, Boris Burkov wrote: > > Pull creating the qgroup earlier in the snapshot. This allows simple > > quotas qgroups to see all the metadata writes related to the snapshot > > being created and to be born with the root node accounted. > > > > Signed-off-by: Boris Burkov <boris@bur.io> > > --- > > fs/btrfs/qgroup.c | 3 +++ > > fs/btrfs/transaction.c | 5 +++++ > > 2 files changed, 8 insertions(+) > > > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > > index 75afd8212bc0..8419270f7417 100644 > > --- a/fs/btrfs/qgroup.c > > +++ b/fs/btrfs/qgroup.c > > @@ -1670,6 +1670,9 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid) > > struct btrfs_qgroup *qgroup; > > int ret = 0; > > > > + if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED) > > + return 0; > > + > > mutex_lock(&fs_info->qgroup_ioctl_lock); > > if (!fs_info->quota_root) { > > ret = -ENOTCONN; > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > > index f644c7c04d53..2bb5a64f6d84 100644 > > --- a/fs/btrfs/transaction.c > > +++ b/fs/btrfs/transaction.c > > @@ -1716,6 +1716,11 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > > } > > btrfs_release_path(path); > > > > + ret = btrfs_create_qgroup(trans, objectid); > > + if (ret) { > > + btrfs_abort_transaction(trans, ret); > > + goto fail; > > + } > > Newline please. > > How is this ok with normal qgroups? We weren't creating a qgroup at snapshot > creation time at all it seems, so I don't understand how this is ok for qgroups. qgroup_account_snapshot calls btrfs_qgroup_inherit which contains a separate implementation of qgroup creation. > Thanks, > > Josef
On Thu, Jul 13, 2023 at 12:00:42PM -0700, Boris Burkov wrote: > On Thu, Jul 13, 2023 at 10:26:00AM -0400, Josef Bacik wrote: > > On Wed, Jul 05, 2023 at 04:20:44PM -0700, Boris Burkov wrote: > > > Pull creating the qgroup earlier in the snapshot. This allows simple > > > quotas qgroups to see all the metadata writes related to the snapshot > > > being created and to be born with the root node accounted. > > > > > > Signed-off-by: Boris Burkov <boris@bur.io> > > > --- > > > fs/btrfs/qgroup.c | 3 +++ > > > fs/btrfs/transaction.c | 5 +++++ > > > 2 files changed, 8 insertions(+) > > > > > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > > > index 75afd8212bc0..8419270f7417 100644 > > > --- a/fs/btrfs/qgroup.c > > > +++ b/fs/btrfs/qgroup.c > > > @@ -1670,6 +1670,9 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid) > > > struct btrfs_qgroup *qgroup; > > > int ret = 0; > > > > > > + if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED) > > > + return 0; > > > + > > > mutex_lock(&fs_info->qgroup_ioctl_lock); > > > if (!fs_info->quota_root) { > > > ret = -ENOTCONN; > > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > > > index f644c7c04d53..2bb5a64f6d84 100644 > > > --- a/fs/btrfs/transaction.c > > > +++ b/fs/btrfs/transaction.c > > > @@ -1716,6 +1716,11 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > > > } > > > btrfs_release_path(path); > > > > > > + ret = btrfs_create_qgroup(trans, objectid); > > > + if (ret) { > > > + btrfs_abort_transaction(trans, ret); > > > + goto fail; > > > + } > > > > Newline please. > > > > How is this ok with normal qgroups? We weren't creating a qgroup at snapshot > > creation time at all it seems, so I don't understand how this is ok for qgroups. > > qgroup_account_snapshot calls btrfs_qgroup_inherit which contains a > separate implementation of qgroup creation. Which it still does, so I'm confused as to how this is ok. How do we not get an EEXIST when we do the btrfs_qgroup_inherit? Thanks, Josef
On Thu, Jul 13, 2023 at 04:37:04PM -0400, Josef Bacik wrote: > On Thu, Jul 13, 2023 at 12:00:42PM -0700, Boris Burkov wrote: > > On Thu, Jul 13, 2023 at 10:26:00AM -0400, Josef Bacik wrote: > > > On Wed, Jul 05, 2023 at 04:20:44PM -0700, Boris Burkov wrote: > > > > Pull creating the qgroup earlier in the snapshot. This allows simple > > > > quotas qgroups to see all the metadata writes related to the snapshot > > > > being created and to be born with the root node accounted. > > > > > > > > Signed-off-by: Boris Burkov <boris@bur.io> > > > > --- > > > > fs/btrfs/qgroup.c | 3 +++ > > > > fs/btrfs/transaction.c | 5 +++++ > > > > 2 files changed, 8 insertions(+) > > > > > > > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > > > > index 75afd8212bc0..8419270f7417 100644 > > > > --- a/fs/btrfs/qgroup.c > > > > +++ b/fs/btrfs/qgroup.c > > > > @@ -1670,6 +1670,9 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid) > > > > struct btrfs_qgroup *qgroup; > > > > int ret = 0; > > > > > > > > + if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED) > > > > + return 0; > > > > + > > > > mutex_lock(&fs_info->qgroup_ioctl_lock); > > > > if (!fs_info->quota_root) { > > > > ret = -ENOTCONN; > > > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > > > > index f644c7c04d53..2bb5a64f6d84 100644 > > > > --- a/fs/btrfs/transaction.c > > > > +++ b/fs/btrfs/transaction.c > > > > @@ -1716,6 +1716,11 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > > > > } > > > > btrfs_release_path(path); > > > > > > > > + ret = btrfs_create_qgroup(trans, objectid); > > > > + if (ret) { > > > > + btrfs_abort_transaction(trans, ret); > > > > + goto fail; > > > > + } > > > > > > Newline please. > > > > > > How is this ok with normal qgroups? We weren't creating a qgroup at snapshot > > > creation time at all it seems, so I don't understand how this is ok for qgroups. > > > > qgroup_account_snapshot calls btrfs_qgroup_inherit which contains a > > separate implementation of qgroup creation. > > Which it still does, so I'm confused as to how this is ok. How do we not get an > EEXIST when we do the btrfs_qgroup_inherit? Thanks, > > Josef add_qgroup_item ignores EEXIST. I believe this should be exercised by a user intentionally creating a qgroup for a subvolume before creating the subvolume (I believe that is explicitly supported, though it does seem to rely on guessing the subvol ID?!) from man btrfs-qgroup: "For the 0/<subvolume id> qgroup, a qgroup can be created even before the subvolume is created."
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 75afd8212bc0..8419270f7417 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1670,6 +1670,9 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid) struct btrfs_qgroup *qgroup; int ret = 0; + if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED) + return 0; + mutex_lock(&fs_info->qgroup_ioctl_lock); if (!fs_info->quota_root) { ret = -ENOTCONN; diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index f644c7c04d53..2bb5a64f6d84 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1716,6 +1716,11 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, } btrfs_release_path(path); + ret = btrfs_create_qgroup(trans, objectid); + if (ret) { + btrfs_abort_transaction(trans, ret); + goto fail; + } /* * pull in the delayed directory update * and the delayed inode item
Pull creating the qgroup earlier in the snapshot. This allows simple quotas qgroups to see all the metadata writes related to the snapshot being created and to be born with the root node accounted. Signed-off-by: Boris Burkov <boris@bur.io> --- fs/btrfs/qgroup.c | 3 +++ fs/btrfs/transaction.c | 5 +++++ 2 files changed, 8 insertions(+)