| Message ID | 20230724142243.5742-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] btrfs: fix error handling when in a COW window in run_delalloc_nocow | expand |
On Mon, Jul 24, 2023 at 07:22:40AM -0700, Christoph Hellwig wrote: > Share the calls to extent_clear_unlock_delalloc for btrfs_path allocation > failure handling and the normal exit path. > > This relies on btrfs_free_path ignoring a NULL pointer, and the > initialization of cur_offset to start at the beginning of the function. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/inode.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 212aca4eea442b..2d465b50c228ed 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -1973,21 +1973,14 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, > struct btrfs_path *path; > u64 cow_start = (u64)-1; > u64 cur_offset = start; > - int ret; > + int ret = -ENOMEM; > bool check_prev = true; > u64 ino = btrfs_ino(inode); > struct can_nocow_file_extent_args nocow_args = { 0 }; > > path = btrfs_alloc_path(); > - if (!path) { > - extent_clear_unlock_delalloc(inode, start, end, locked_page, > - EXTENT_LOCKED | EXTENT_DELALLOC | > - EXTENT_DO_ACCOUNTING | > - EXTENT_DEFRAG, PAGE_UNLOCK | > - PAGE_START_WRITEBACK | > - PAGE_END_WRITEBACK); > - return -ENOMEM; > - } > + if (!path) > + goto error; nit: I think it's nicer to do ret = -ENOMEM here rather than relying on initializion. It makes it less likely for a different change to accidentally disrupt the implicit assumption that ret == -ENOMEM. > > nocow_args.end = end; > nocow_args.writeback_path = true; > -- > 2.39.2 >
On Mon, Jul 24, 2023 at 11:27:37AM -0700, Boris Burkov wrote: > > - return -ENOMEM; > > - } > > + if (!path) > > + goto error; > > nit: I think it's nicer to do ret = -ENOMEM here rather than relying on > initializion. It makes it less likely for a different change to > accidentally disrupt the implicit assumption that ret == -ENOMEM. I kinda like the early initialization version, but I can live with either variant.
On Mon, Jul 24, 2023 at 09:48:44PM +0200, Christoph Hellwig wrote: > On Mon, Jul 24, 2023 at 11:27:37AM -0700, Boris Burkov wrote: > > > - return -ENOMEM; > > > - } > > > + if (!path) > > > + goto error; > > > > nit: I think it's nicer to do ret = -ENOMEM here rather than relying on > > initializion. It makes it less likely for a different change to > > accidentally disrupt the implicit assumption that ret == -ENOMEM. > > I kinda like the early initialization version, but I can live with > either variant. We use the style where the ret is initialized to 0 and then either there's a direct return of error code or 'ret = -ERROR' followed by a goto. There are a few remaining to convert but please don't add new ones.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 212aca4eea442b..2d465b50c228ed 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -1973,21 +1973,14 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, struct btrfs_path *path; u64 cow_start = (u64)-1; u64 cur_offset = start; - int ret; + int ret = -ENOMEM; bool check_prev = true; u64 ino = btrfs_ino(inode); struct can_nocow_file_extent_args nocow_args = { 0 }; path = btrfs_alloc_path(); - if (!path) { - extent_clear_unlock_delalloc(inode, start, end, locked_page, - EXTENT_LOCKED | EXTENT_DELALLOC | - EXTENT_DO_ACCOUNTING | - EXTENT_DEFRAG, PAGE_UNLOCK | - PAGE_START_WRITEBACK | - PAGE_END_WRITEBACK); - return -ENOMEM; - } + if (!path) + goto error; nocow_args.end = end; nocow_args.writeback_path = true;
Share the calls to extent_clear_unlock_delalloc for btrfs_path allocation failure handling and the normal exit path. This relies on btrfs_free_path ignoring a NULL pointer, and the initialization of cur_offset to start at the beginning of the function. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/inode.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)