| Message ID | 20221205095122.17011-4-robbieko@synology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix unexpected -ENOMEM with percpu_counter_init when create snapshot | expand |
On Mon, Dec 5, 2022 at 10:36 AM robbieko <robbieko@synology.com> wrote: > > From: Robbie Ko <robbieko@synology.com> > > Add snapshot_lock into new_fs_root_args to prevent > percpu_counter_init enter unexpected -ENOMEM when > calling btrfs_init_fs_root. You could be more detailed here and mention that all this is to prevent transaction aborts in case percpu_counter_init() fails to allocate memory. Interpreting literally what you wrote, it gives the idea that after this patch the memory allocation can never fail, which isn't true. The goal is just to allocate the snapshot lock before holding a transaction handle, so that we can use GFP_KERNEL, which reduces the chances of memory allocation failing and, above all, avoid aborting a transaction and turn the fs to RO mode. > > Signed-off-by: Robbie Ko <robbieko@synology.com> > --- > fs/btrfs/disk-io.c | 44 +++++++++++++++++++++++++++++++------------- > fs/btrfs/disk-io.h | 2 ++ > 2 files changed, 33 insertions(+), 13 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 5760c2b1a100..5704c8f5873c 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -1437,6 +1437,16 @@ struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask) > if (err) > goto error; > > + args->snapshot_lock = kzalloc(sizeof(*args->snapshot_lock), gfp_mask); > + if (!args->snapshot_lock) { > + err = -ENOMEM; > + goto error; > + } > + ASSERT((gfp_mask & GFP_KERNEL) == GFP_KERNEL); As commented for patch 1/3, we could get away with the argument and directly use GFP_KERNEL above, and let lockdep warn us (which makes fstests fail). > + err = btrfs_drew_lock_init(args->snapshot_lock); > + if (err) > + goto error; > + > return args; > > error: > @@ -1448,6 +1458,9 @@ void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args) > { > if (!args) > return; > + if (args->snapshot_lock) > + btrfs_drew_lock_destroy(args->snapshot_lock); > + kfree(args->snapshot_lock); You could combine the kfree of the lock inside the if statement, making it more clear imo. > if (args->anon_dev) > free_anon_bdev(args->anon_dev); > kfree(args); > @@ -1464,20 +1477,25 @@ static int btrfs_init_fs_root(struct btrfs_root *root, > int ret; > unsigned int nofs_flag; > > - root->snapshot_lock = kzalloc(sizeof(*root->snapshot_lock), GFP_NOFS); > - if (!root->snapshot_lock) { > - ret = -ENOMEM; > - goto fail; > + if (new_fs_root_args && new_fs_root_args->snapshot_lock) { > + root->snapshot_lock = new_fs_root_args->snapshot_lock; > + new_fs_root_args->snapshot_lock = NULL; > + } else { > + root->snapshot_lock = kzalloc(sizeof(*root->snapshot_lock), GFP_NOFS); > + if (!root->snapshot_lock) { > + ret = -ENOMEM; > + goto fail; > + } > + /* > + * We might be called under a transaction (e.g. indirect backref > + * resolution) which could deadlock if it triggers memory reclaim While here, we could end the sentence with punctuation (.). Thanks. > + */ > + nofs_flag = memalloc_nofs_save(); > + ret = btrfs_drew_lock_init(root->snapshot_lock); > + memalloc_nofs_restore(nofs_flag); > + if (ret) > + goto fail; > } > - /* > - * We might be called under a transaction (e.g. indirect backref > - * resolution) which could deadlock if it triggers memory reclaim > - */ > - nofs_flag = memalloc_nofs_save(); > - ret = btrfs_drew_lock_init(root->snapshot_lock); > - memalloc_nofs_restore(nofs_flag); > - if (ret) > - goto fail; > > if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID && > !btrfs_is_data_reloc_root(root)) { > diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h > index a7c91bfb0c16..0e84927859ce 100644 > --- a/fs/btrfs/disk-io.h > +++ b/fs/btrfs/disk-io.h > @@ -28,6 +28,8 @@ static inline u64 btrfs_sb_offset(int mirror) > struct btrfs_new_fs_root_args { > /* Preallocated anonymous block device number */ > dev_t anon_dev; > + /* Preallocated snapshot lock */ > + struct btrfs_drew_lock *snapshot_lock; > }; > > struct btrfs_device; > -- > 2.17.1 >
Filipe Manana 於 2022/12/13 上午1:06 寫道: > On Mon, Dec 5, 2022 at 10:36 AM robbieko <robbieko@synology.com> wrote: >> From: Robbie Ko <robbieko@synology.com> >> >> Add snapshot_lock into new_fs_root_args to prevent >> percpu_counter_init enter unexpected -ENOMEM when >> calling btrfs_init_fs_root. > You could be more detailed here and mention that all this is to > prevent transaction aborts in case percpu_counter_init() fails to > allocate memory. > > Interpreting literally what you wrote, it gives the idea that after > this patch the memory allocation can never fail, which isn't true. > The goal is just to allocate the snapshot lock before holding a > transaction handle, so that we can use GFP_KERNEL, which reduces the > chances of memory allocation failing and, above all, avoid aborting a > transaction and turn the fs to RO mode. Okay. I will fix the commit message to provide a more detailed description. Thanks. > >> Signed-off-by: Robbie Ko <robbieko@synology.com> >> --- >> fs/btrfs/disk-io.c | 44 +++++++++++++++++++++++++++++++------------- >> fs/btrfs/disk-io.h | 2 ++ >> 2 files changed, 33 insertions(+), 13 deletions(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index 5760c2b1a100..5704c8f5873c 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -1437,6 +1437,16 @@ struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask) >> if (err) >> goto error; >> >> + args->snapshot_lock = kzalloc(sizeof(*args->snapshot_lock), gfp_mask); >> + if (!args->snapshot_lock) { >> + err = -ENOMEM; >> + goto error; >> + } >> + ASSERT((gfp_mask & GFP_KERNEL) == GFP_KERNEL); > As commented for patch 1/3, we could get away with the argument and > directly use GFP_KERNEL above, and let lockdep warn us (which makes > fstests fail). Okay. > >> + err = btrfs_drew_lock_init(args->snapshot_lock); >> + if (err) >> + goto error; >> + >> return args; >> >> error: >> @@ -1448,6 +1458,9 @@ void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args) >> { >> if (!args) >> return; >> + if (args->snapshot_lock) >> + btrfs_drew_lock_destroy(args->snapshot_lock); >> + kfree(args->snapshot_lock); > You could combine the kfree of the lock inside the if statement, > making it more clear imo. Okay. > >> if (args->anon_dev) >> free_anon_bdev(args->anon_dev); >> kfree(args); >> @@ -1464,20 +1477,25 @@ static int btrfs_init_fs_root(struct btrfs_root *root, >> int ret; >> unsigned int nofs_flag; >> >> - root->snapshot_lock = kzalloc(sizeof(*root->snapshot_lock), GFP_NOFS); >> - if (!root->snapshot_lock) { >> - ret = -ENOMEM; >> - goto fail; >> + if (new_fs_root_args && new_fs_root_args->snapshot_lock) { >> + root->snapshot_lock = new_fs_root_args->snapshot_lock; >> + new_fs_root_args->snapshot_lock = NULL; >> + } else { >> + root->snapshot_lock = kzalloc(sizeof(*root->snapshot_lock), GFP_NOFS); >> + if (!root->snapshot_lock) { >> + ret = -ENOMEM; >> + goto fail; >> + } >> + /* >> + * We might be called under a transaction (e.g. indirect backref >> + * resolution) which could deadlock if it triggers memory reclaim > While here, we could end the sentence with punctuation (.). Okay. > Thanks. > >> + */ >> + nofs_flag = memalloc_nofs_save(); >> + ret = btrfs_drew_lock_init(root->snapshot_lock); >> + memalloc_nofs_restore(nofs_flag); >> + if (ret) >> + goto fail; >> } >> - /* >> - * We might be called under a transaction (e.g. indirect backref >> - * resolution) which could deadlock if it triggers memory reclaim >> - */ >> - nofs_flag = memalloc_nofs_save(); >> - ret = btrfs_drew_lock_init(root->snapshot_lock); >> - memalloc_nofs_restore(nofs_flag); >> - if (ret) >> - goto fail; >> >> if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID && >> !btrfs_is_data_reloc_root(root)) { >> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h >> index a7c91bfb0c16..0e84927859ce 100644 >> --- a/fs/btrfs/disk-io.h >> +++ b/fs/btrfs/disk-io.h >> @@ -28,6 +28,8 @@ static inline u64 btrfs_sb_offset(int mirror) >> struct btrfs_new_fs_root_args { >> /* Preallocated anonymous block device number */ >> dev_t anon_dev; >> + /* Preallocated snapshot lock */ >> + struct btrfs_drew_lock *snapshot_lock; >> }; >> >> struct btrfs_device; >> -- >> 2.17.1 >>
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 5760c2b1a100..5704c8f5873c 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -1437,6 +1437,16 @@ struct btrfs_new_fs_root_args *btrfs_new_fs_root_args_prepare(gfp_t gfp_mask) if (err) goto error; + args->snapshot_lock = kzalloc(sizeof(*args->snapshot_lock), gfp_mask); + if (!args->snapshot_lock) { + err = -ENOMEM; + goto error; + } + ASSERT((gfp_mask & GFP_KERNEL) == GFP_KERNEL); + err = btrfs_drew_lock_init(args->snapshot_lock); + if (err) + goto error; + return args; error: @@ -1448,6 +1458,9 @@ void btrfs_new_fs_root_args_destroy(struct btrfs_new_fs_root_args *args) { if (!args) return; + if (args->snapshot_lock) + btrfs_drew_lock_destroy(args->snapshot_lock); + kfree(args->snapshot_lock); if (args->anon_dev) free_anon_bdev(args->anon_dev); kfree(args); @@ -1464,20 +1477,25 @@ static int btrfs_init_fs_root(struct btrfs_root *root, int ret; unsigned int nofs_flag; - root->snapshot_lock = kzalloc(sizeof(*root->snapshot_lock), GFP_NOFS); - if (!root->snapshot_lock) { - ret = -ENOMEM; - goto fail; + if (new_fs_root_args && new_fs_root_args->snapshot_lock) { + root->snapshot_lock = new_fs_root_args->snapshot_lock; + new_fs_root_args->snapshot_lock = NULL; + } else { + root->snapshot_lock = kzalloc(sizeof(*root->snapshot_lock), GFP_NOFS); + if (!root->snapshot_lock) { + ret = -ENOMEM; + goto fail; + } + /* + * We might be called under a transaction (e.g. indirect backref + * resolution) which could deadlock if it triggers memory reclaim + */ + nofs_flag = memalloc_nofs_save(); + ret = btrfs_drew_lock_init(root->snapshot_lock); + memalloc_nofs_restore(nofs_flag); + if (ret) + goto fail; } - /* - * We might be called under a transaction (e.g. indirect backref - * resolution) which could deadlock if it triggers memory reclaim - */ - nofs_flag = memalloc_nofs_save(); - ret = btrfs_drew_lock_init(root->snapshot_lock); - memalloc_nofs_restore(nofs_flag); - if (ret) - goto fail; if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID && !btrfs_is_data_reloc_root(root)) { diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h index a7c91bfb0c16..0e84927859ce 100644 --- a/fs/btrfs/disk-io.h +++ b/fs/btrfs/disk-io.h @@ -28,6 +28,8 @@ static inline u64 btrfs_sb_offset(int mirror) struct btrfs_new_fs_root_args { /* Preallocated anonymous block device number */ dev_t anon_dev; + /* Preallocated snapshot lock */ + struct btrfs_drew_lock *snapshot_lock; }; struct btrfs_device;