| Message ID | 20190225185544.14516-1-dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: move ulist allocation out of transaction in quota enable | expand |
On Mon, Feb 25, 2019 at 6:58 PM David Sterba <dsterba@suse.com> wrote: > > The allocation happens with GFP_KERNEL after a transaction has been > started, this can potentially cause deadlock if reclaim tries to get the > memory by flushing filesystem data. > > The qgroup_ulist is not used during transaction start as the quotas are > not yet enabled but to provide the same semantics, use a temporary > variable and assign fs_info::qgroup_ulist as before. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/qgroup.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index c1cd5558a646..9e49d5e132b8 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -887,6 +887,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) > struct btrfs_key found_key; > struct btrfs_qgroup *qgroup = NULL; > struct btrfs_trans_handle *trans = NULL; > + struct ulist *tmp_ulist; > int ret = 0; > int slot; > > @@ -894,6 +895,12 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) > if (fs_info->quota_root) > goto out; > > + tmp_ulist = ulist_alloc(GFP_KERNEL); > + if (!tmp_ulist) { > + ret = -ENOMEM; > + goto out; > + } tmp_ulist needs to be freed if starting the transaction below fails. Other then that, looks good. > + > /* > * 1 for quota root item > * 1 for BTRFS_QGROUP_STATUS item > @@ -909,12 +916,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) > goto out; > } > > - fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL); > - if (!fs_info->qgroup_ulist) { > - ret = -ENOMEM; > - btrfs_abort_transaction(trans, ret); > - goto out; > - } > + fs_info->qgroup_ulist = tmp_ulist; > > /* > * initially create the quota tree > -- > 2.20.1 >
On Mon, Feb 25, 2019 at 07:09:02PM +0000, Filipe Manana wrote: > On Mon, Feb 25, 2019 at 6:58 PM David Sterba <dsterba@suse.com> wrote: > > > > The allocation happens with GFP_KERNEL after a transaction has been > > started, this can potentially cause deadlock if reclaim tries to get the > > memory by flushing filesystem data. > > > > The qgroup_ulist is not used during transaction start as the quotas are > > not yet enabled but to provide the same semantics, use a temporary > > variable and assign fs_info::qgroup_ulist as before. > > > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/qgroup.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > > index c1cd5558a646..9e49d5e132b8 100644 > > --- a/fs/btrfs/qgroup.c > > +++ b/fs/btrfs/qgroup.c > > @@ -887,6 +887,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) > > struct btrfs_key found_key; > > struct btrfs_qgroup *qgroup = NULL; > > struct btrfs_trans_handle *trans = NULL; > > + struct ulist *tmp_ulist; > > int ret = 0; > > int slot; > > > > @@ -894,6 +895,12 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) > > if (fs_info->quota_root) > > goto out; > > > > + tmp_ulist = ulist_alloc(GFP_KERNEL); > > + if (!tmp_ulist) { > > + ret = -ENOMEM; > > + goto out; > > + } > > tmp_ulist needs to be freed if starting the transaction below fails. Right. If I did not try to preserve the semantics (that does not seem to be necessary), the freeing would happen on qgroup_ulist as before. Starting transaction uses qgruop_ulist if quotas are enabled, so at this time of btrfs_quota_enable, this is not yet true. The status bit is set later. I'll update the patch with this explanation and drop the temporary variable.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index c1cd5558a646..9e49d5e132b8 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -887,6 +887,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) struct btrfs_key found_key; struct btrfs_qgroup *qgroup = NULL; struct btrfs_trans_handle *trans = NULL; + struct ulist *tmp_ulist; int ret = 0; int slot; @@ -894,6 +895,12 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) if (fs_info->quota_root) goto out; + tmp_ulist = ulist_alloc(GFP_KERNEL); + if (!tmp_ulist) { + ret = -ENOMEM; + goto out; + } + /* * 1 for quota root item * 1 for BTRFS_QGROUP_STATUS item @@ -909,12 +916,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) goto out; } - fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL); - if (!fs_info->qgroup_ulist) { - ret = -ENOMEM; - btrfs_abort_transaction(trans, ret); - goto out; - } + fs_info->qgroup_ulist = tmp_ulist; /* * initially create the quota tree
The allocation happens with GFP_KERNEL after a transaction has been started, this can potentially cause deadlock if reclaim tries to get the memory by flushing filesystem data. The qgroup_ulist is not used during transaction start as the quotas are not yet enabled but to provide the same semantics, use a temporary variable and assign fs_info::qgroup_ulist as before. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/qgroup.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)