* [PATCH] btrfs: move ulist allocation out of transaction in quota enable
@ 2019-02-25 18:55 David Sterba
2019-02-25 19:09 ` Filipe Manana
2019-02-27 12:13 ` [PATCH v2] " David Sterba
0 siblings, 2 replies; 5+ messages in thread
From: David Sterba @ 2019-02-25 18:55 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
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;
+ }
+
/*
* 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: move ulist allocation out of transaction in quota enable
2019-02-25 18:55 [PATCH] btrfs: move ulist allocation out of transaction in quota enable David Sterba
@ 2019-02-25 19:09 ` Filipe Manana
2019-02-27 12:06 ` David Sterba
2019-02-27 12:13 ` [PATCH v2] " David Sterba
1 sibling, 1 reply; 5+ messages in thread
From: Filipe Manana @ 2019-02-25 19:09 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
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
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: move ulist allocation out of transaction in quota enable
2019-02-25 19:09 ` Filipe Manana
@ 2019-02-27 12:06 ` David Sterba
0 siblings, 0 replies; 5+ messages in thread
From: David Sterba @ 2019-02-27 12:06 UTC (permalink / raw)
To: Filipe Manana; +Cc: David Sterba, linux-btrfs
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] btrfs: move ulist allocation out of transaction in quota enable
2019-02-25 18:55 [PATCH] btrfs: move ulist allocation out of transaction in quota enable David Sterba
2019-02-25 19:09 ` Filipe Manana
@ 2019-02-27 12:13 ` David Sterba
2019-02-27 12:34 ` Filipe Manana
1 sibling, 1 reply; 5+ messages in thread
From: David Sterba @ 2019-02-27 12:13 UTC (permalink / raw)
To: linux-btrfs; +Cc: fdmanana, David Sterba
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 fs_info::qgroup_ulist is not used during transaction start when
quotas are not enabled. The status bit BTRFS_FS_QUOTA_ENABLED is set
later in btrfs_quota_enable so it's safe to move it before the
transaction start.
Signed-off-by: David Sterba <dsterba@suse.com>
v2:
- use qgroup_ulist directly, drop the temporary variable
- update changelog
---
fs/btrfs/qgroup.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index c1cd5558a646..eb680b715dd6 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -894,6 +894,12 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
if (fs_info->quota_root)
goto out;
+ fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
+ if (!fs_info->qgroup_ulist) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
/*
* 1 for quota root item
* 1 for BTRFS_QGROUP_STATUS item
@@ -909,13 +915,6 @@ 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;
- }
-
/*
* initially create the quota tree
*/
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: move ulist allocation out of transaction in quota enable
2019-02-27 12:13 ` [PATCH v2] " David Sterba
@ 2019-02-27 12:34 ` Filipe Manana
0 siblings, 0 replies; 5+ messages in thread
From: Filipe Manana @ 2019-02-27 12:34 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Wed, Feb 27, 2019 at 12:12 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 fs_info::qgroup_ulist is not used during transaction start when
> quotas are not enabled. The status bit BTRFS_FS_QUOTA_ENABLED is set
> later in btrfs_quota_enable so it's safe to move it before the
> transaction start.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Looks good now.
>
> v2:
> - use qgroup_ulist directly, drop the temporary variable
> - update changelog
> ---
> fs/btrfs/qgroup.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index c1cd5558a646..eb680b715dd6 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -894,6 +894,12 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> if (fs_info->quota_root)
> goto out;
>
> + fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
> + if (!fs_info->qgroup_ulist) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> /*
> * 1 for quota root item
> * 1 for BTRFS_QGROUP_STATUS item
> @@ -909,13 +915,6 @@ 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;
> - }
> -
> /*
> * initially create the quota tree
> */
> --
> 2.20.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-27 12:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-25 18:55 [PATCH] btrfs: move ulist allocation out of transaction in quota enable David Sterba
2019-02-25 19:09 ` Filipe Manana
2019-02-27 12:06 ` David Sterba
2019-02-27 12:13 ` [PATCH v2] " David Sterba
2019-02-27 12:34 ` Filipe Manana
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).