| Message ID | 20180719145006.17532-10-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19.07.2018 17:49, Josef Bacik wrote: > If we're allocating a new space cache inode it's likely going to be > under a transaction handle, so we need to use GFP_NOFS to keep from > deadlocking. Otherwise GFP_KERNEL is fine. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/free-space-cache.c | 5 +++++ > fs/btrfs/inode.c | 3 ++- > fs/btrfs/transaction.h | 1 + > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > index d5f80cb300be..13bc514e4e16 100644 > --- a/fs/btrfs/free-space-cache.c > +++ b/fs/btrfs/free-space-cache.c > @@ -68,7 +68,12 @@ static struct inode *__lookup_free_space_inode(struct btrfs_root *root, > btrfs_disk_key_to_cpu(&location, &disk_key); > btrfs_release_path(path); > > + /* We need this set so that we use GFP_NOFS when allocating our inode. */ > + if (current->journal_info == NULL) > + current->journal_info = BTRFS_TRANS_STUB; > inode = btrfs_iget(fs_info->sb, &location, root, NULL); > + if (current->journal_info == BTRFS_TRANS_STUB) > + current->journal_info = NULL; This is not safe in the face of stacked filesystem, i.e ext4 uses the journal_info. > if (IS_ERR(inode)) > return inode; > if (is_bad_inode(inode)) { > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index eba61bcb9bb3..14ecfe5d6110 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -9211,8 +9211,9 @@ struct inode *btrfs_alloc_inode(struct super_block *sb) > struct btrfs_fs_info *fs_info = btrfs_sb(sb); > struct btrfs_inode *ei; > struct inode *inode; > + gfp_t flags = (current->journal_info) ? GFP_NOFS : GFP_KERNEL; > > - ei = kmem_cache_alloc(btrfs_inode_cachep, GFP_KERNEL); > + ei = kmem_cache_alloc(btrfs_inode_cachep, flags); Why don't you just hardcode GFP_NOFS? We should be striving at removing abuse of ->journal_info than proliferating it. > if (!ei) > return NULL; > > diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h > index 94439482a0ec..172ff923bf15 100644 > --- a/fs/btrfs/transaction.h > +++ b/fs/btrfs/transaction.h > @@ -106,6 +106,7 @@ struct btrfs_transaction { > #define TRANS_EXTWRITERS (__TRANS_START | __TRANS_ATTACH) > > #define BTRFS_SEND_TRANS_STUB ((void *)1) > +#define BTRFS_TRANS_STUB ((void *)2) > > struct btrfs_trans_handle { > u64 transid; > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 19, 2018 at 06:35:33PM +0300, Nikolay Borisov wrote: > > > On 19.07.2018 17:49, Josef Bacik wrote: > > If we're allocating a new space cache inode it's likely going to be > > under a transaction handle, so we need to use GFP_NOFS to keep from > > deadlocking. Otherwise GFP_KERNEL is fine. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/btrfs/free-space-cache.c | 5 +++++ > > fs/btrfs/inode.c | 3 ++- > > fs/btrfs/transaction.h | 1 + > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > index d5f80cb300be..13bc514e4e16 100644 > > --- a/fs/btrfs/free-space-cache.c > > +++ b/fs/btrfs/free-space-cache.c > > @@ -68,7 +68,12 @@ static struct inode *__lookup_free_space_inode(struct btrfs_root *root, > > btrfs_disk_key_to_cpu(&location, &disk_key); > > btrfs_release_path(path); > > > > + /* We need this set so that we use GFP_NOFS when allocating our inode. */ > > + if (current->journal_info == NULL) > > + current->journal_info = BTRFS_TRANS_STUB; > > inode = btrfs_iget(fs_info->sb, &location, root, NULL); > > + if (current->journal_info == BTRFS_TRANS_STUB) > > + current->journal_info = NULL; > This is not safe in the face of stacked filesystem, i.e ext4 uses the > journal_info. > > > if (IS_ERR(inode)) > > return inode; > > if (is_bad_inode(inode)) { > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index eba61bcb9bb3..14ecfe5d6110 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -9211,8 +9211,9 @@ struct inode *btrfs_alloc_inode(struct super_block *sb) > > struct btrfs_fs_info *fs_info = btrfs_sb(sb); > > struct btrfs_inode *ei; > > struct inode *inode; > > + gfp_t flags = (current->journal_info) ? GFP_NOFS : GFP_KERNEL; > > > > - ei = kmem_cache_alloc(btrfs_inode_cachep, GFP_KERNEL); > > + ei = kmem_cache_alloc(btrfs_inode_cachep, flags); > > Why don't you just hardcode GFP_NOFS? We should be striving at removing > abuse of ->journal_info than proliferating it. We're also removing unnecessary use of GFP_NOFS. The right way here is to use memalloc_nofs_save/memalloc_nofs_restore around btrfs_iget. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 19, 2018 at 06:35:33PM +0300, Nikolay Borisov wrote: > > > On 19.07.2018 17:49, Josef Bacik wrote: > > If we're allocating a new space cache inode it's likely going to be > > under a transaction handle, so we need to use GFP_NOFS to keep from > > deadlocking. Otherwise GFP_KERNEL is fine. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/btrfs/free-space-cache.c | 5 +++++ > > fs/btrfs/inode.c | 3 ++- > > fs/btrfs/transaction.h | 1 + > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > index d5f80cb300be..13bc514e4e16 100644 > > --- a/fs/btrfs/free-space-cache.c > > +++ b/fs/btrfs/free-space-cache.c > > @@ -68,7 +68,12 @@ static struct inode *__lookup_free_space_inode(struct btrfs_root *root, > > btrfs_disk_key_to_cpu(&location, &disk_key); > > btrfs_release_path(path); > > > > + /* We need this set so that we use GFP_NOFS when allocating our inode. */ > > + if (current->journal_info == NULL) > > + current->journal_info = BTRFS_TRANS_STUB; > > inode = btrfs_iget(fs_info->sb, &location, root, NULL); > > + if (current->journal_info == BTRFS_TRANS_STUB) > > + current->journal_info = NULL; > This is not safe in the face of stacked filesystem, i.e ext4 uses the > journal_info. Stacked file systems arent safe at all because we all use journal_info, this doesn't change that. > > > if (IS_ERR(inode)) > > return inode; > > if (is_bad_inode(inode)) { > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index eba61bcb9bb3..14ecfe5d6110 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -9211,8 +9211,9 @@ struct inode *btrfs_alloc_inode(struct super_block *sb) > > struct btrfs_fs_info *fs_info = btrfs_sb(sb); > > struct btrfs_inode *ei; > > struct inode *inode; > > + gfp_t flags = (current->journal_info) ? GFP_NOFS : GFP_KERNEL; > > > > - ei = kmem_cache_alloc(btrfs_inode_cachep, GFP_KERNEL); > > + ei = kmem_cache_alloc(btrfs_inode_cachep, flags); > > Why don't you just hardcode GFP_NOFS? We should be striving at removing > abuse of ->journal_info than proliferating it. > Because every time I use GFP_NOFS some mm guy shows up and complains. I'm fine with doing GFP_NOFS, but the vast majority of allocations are in paths that GFP_KERNEL is fine. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 19, 2018 at 05:44:53PM +0200, David Sterba wrote: > On Thu, Jul 19, 2018 at 06:35:33PM +0300, Nikolay Borisov wrote: > > > > > > On 19.07.2018 17:49, Josef Bacik wrote: > > > If we're allocating a new space cache inode it's likely going to be > > > under a transaction handle, so we need to use GFP_NOFS to keep from > > > deadlocking. Otherwise GFP_KERNEL is fine. > > > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > --- > > > fs/btrfs/free-space-cache.c | 5 +++++ > > > fs/btrfs/inode.c | 3 ++- > > > fs/btrfs/transaction.h | 1 + > > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > > index d5f80cb300be..13bc514e4e16 100644 > > > --- a/fs/btrfs/free-space-cache.c > > > +++ b/fs/btrfs/free-space-cache.c > > > @@ -68,7 +68,12 @@ static struct inode *__lookup_free_space_inode(struct btrfs_root *root, > > > btrfs_disk_key_to_cpu(&location, &disk_key); > > > btrfs_release_path(path); > > > > > > + /* We need this set so that we use GFP_NOFS when allocating our inode. */ > > > + if (current->journal_info == NULL) > > > + current->journal_info = BTRFS_TRANS_STUB; > > > inode = btrfs_iget(fs_info->sb, &location, root, NULL); > > > + if (current->journal_info == BTRFS_TRANS_STUB) > > > + current->journal_info = NULL; > > This is not safe in the face of stacked filesystem, i.e ext4 uses the > > journal_info. > > > > > if (IS_ERR(inode)) > > > return inode; > > > if (is_bad_inode(inode)) { > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > > index eba61bcb9bb3..14ecfe5d6110 100644 > > > --- a/fs/btrfs/inode.c > > > +++ b/fs/btrfs/inode.c > > > @@ -9211,8 +9211,9 @@ struct inode *btrfs_alloc_inode(struct super_block *sb) > > > struct btrfs_fs_info *fs_info = btrfs_sb(sb); > > > struct btrfs_inode *ei; > > > struct inode *inode; > > > + gfp_t flags = (current->journal_info) ? GFP_NOFS : GFP_KERNEL; > > > > > > - ei = kmem_cache_alloc(btrfs_inode_cachep, GFP_KERNEL); > > > + ei = kmem_cache_alloc(btrfs_inode_cachep, flags); > > > > Why don't you just hardcode GFP_NOFS? We should be striving at removing > > abuse of ->journal_info than proliferating it. > > We're also removing unnecessary use of GFP_NOFS. The right way here is > to use memalloc_nofs_save/memalloc_nofs_restore around btrfs_iget. Huh I didn't know this existed, that's neat. I'll change this patch to do that, thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c index d5f80cb300be..13bc514e4e16 100644 --- a/fs/btrfs/free-space-cache.c +++ b/fs/btrfs/free-space-cache.c @@ -68,7 +68,12 @@ static struct inode *__lookup_free_space_inode(struct btrfs_root *root, btrfs_disk_key_to_cpu(&location, &disk_key); btrfs_release_path(path); + /* We need this set so that we use GFP_NOFS when allocating our inode. */ + if (current->journal_info == NULL) + current->journal_info = BTRFS_TRANS_STUB; inode = btrfs_iget(fs_info->sb, &location, root, NULL); + if (current->journal_info == BTRFS_TRANS_STUB) + current->journal_info = NULL; if (IS_ERR(inode)) return inode; if (is_bad_inode(inode)) { diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index eba61bcb9bb3..14ecfe5d6110 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -9211,8 +9211,9 @@ struct inode *btrfs_alloc_inode(struct super_block *sb) struct btrfs_fs_info *fs_info = btrfs_sb(sb); struct btrfs_inode *ei; struct inode *inode; + gfp_t flags = (current->journal_info) ? GFP_NOFS : GFP_KERNEL; - ei = kmem_cache_alloc(btrfs_inode_cachep, GFP_KERNEL); + ei = kmem_cache_alloc(btrfs_inode_cachep, flags); if (!ei) return NULL; diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h index 94439482a0ec..172ff923bf15 100644 --- a/fs/btrfs/transaction.h +++ b/fs/btrfs/transaction.h @@ -106,6 +106,7 @@ struct btrfs_transaction { #define TRANS_EXTWRITERS (__TRANS_START | __TRANS_ATTACH) #define BTRFS_SEND_TRANS_STUB ((void *)1) +#define BTRFS_TRANS_STUB ((void *)2) struct btrfs_trans_handle { u64 transid;
If we're allocating a new space cache inode it's likely going to be under a transaction handle, so we need to use GFP_NOFS to keep from deadlocking. Otherwise GFP_KERNEL is fine. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/free-space-cache.c | 5 +++++ fs/btrfs/inode.c | 3 ++- fs/btrfs/transaction.h | 1 + 3 files changed, 8 insertions(+), 1 deletion(-)