| Message ID | 60f8e6362e50086d796d8cdd44031d9496398b08.1713363472.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: restrain lock extent usage during writeback | expand |
On 10:35 17/04, Josef Bacik wrote: > run_delalloc_nocow is a bit special as it walks through the file extents > for the inode and determines what it can nocow and what it can't. This > is the more complicated area for extent locking, so start with this > function. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/inode.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 2083005f2828..f14b3cecce47 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, > */ > ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root)); > > + lock_extent(&inode->io_tree, start, end, NULL); > + > path = btrfs_alloc_path(); > if (!path) { > ret = -ENOMEM; > @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page > const bool zoned = btrfs_is_zoned(inode->root->fs_info); > int ret; > > - /* > - * We're unlocked by the different fill functions below. > - */ > - lock_extent(&inode->io_tree, start, end, NULL); > - So, you are adding this hunk in the previous patch and (re)moving it here. Do you think it would be better to merge this with the previous patch? > /* > * The range must cover part of the @locked_page, or a return of 1 > * can confuse the caller. > @@ -2266,6 +2263,11 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page > goto out; > } > > + /* > + * We're unlocked by the different fill functions below. > + */ > + lock_extent(&inode->io_tree, start, end, NULL); > + > if (btrfs_inode_can_compress(inode) && > inode_need_compress(inode, start, end) && > run_delalloc_compressed(inode, locked_page, start, end, wbc)) > -- > 2.43.0 > >
On Tue, Apr 23, 2024 at 06:33:25AM -0500, Goldwyn Rodrigues wrote: > On 10:35 17/04, Josef Bacik wrote: > > run_delalloc_nocow is a bit special as it walks through the file extents > > for the inode and determines what it can nocow and what it can't. This > > is the more complicated area for extent locking, so start with this > > function. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > > --- > > fs/btrfs/inode.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 2083005f2828..f14b3cecce47 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, > > */ > > ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root)); > > > > + lock_extent(&inode->io_tree, start, end, NULL); > > + > > path = btrfs_alloc_path(); > > if (!path) { > > ret = -ENOMEM; > > @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page > > const bool zoned = btrfs_is_zoned(inode->root->fs_info); > > int ret; > > > > - /* > > - * We're unlocked by the different fill functions below. > > - */ > > - lock_extent(&inode->io_tree, start, end, NULL); > > - > > So, you are adding this hunk in the previous patch and (re)moving it here. > Do you think it would be better to merge this with the previous patch? It depends? I did it like this so people could follow closely as I pushed the locking down. Most of these "push extent lock into.." patches do a variation of this, where I push it down into a function and move the lock down in the set of things. I'm happy to merge them together, but I split it this way so my logic could be followed. Thanks, Josef
On Tue, Apr 23, 2024 at 12:49:14PM -0400, Josef Bacik wrote: > On Tue, Apr 23, 2024 at 06:33:25AM -0500, Goldwyn Rodrigues wrote: > > On 10:35 17/04, Josef Bacik wrote: > > > run_delalloc_nocow is a bit special as it walks through the file extents > > > for the inode and determines what it can nocow and what it can't. This > > > is the more complicated area for extent locking, so start with this > > > function. > > > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > > > > > --- > > > fs/btrfs/inode.c | 12 +++++++----- > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > > index 2083005f2828..f14b3cecce47 100644 > > > --- a/fs/btrfs/inode.c > > > +++ b/fs/btrfs/inode.c > > > @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, > > > */ > > > ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root)); > > > > > > + lock_extent(&inode->io_tree, start, end, NULL); > > > + > > > path = btrfs_alloc_path(); > > > if (!path) { > > > ret = -ENOMEM; > > > @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page > > > const bool zoned = btrfs_is_zoned(inode->root->fs_info); > > > int ret; > > > > > > - /* > > > - * We're unlocked by the different fill functions below. > > > - */ > > > - lock_extent(&inode->io_tree, start, end, NULL); > > > - > > > > So, you are adding this hunk in the previous patch and (re)moving it here. > > Do you think it would be better to merge this with the previous patch? > > It depends? I did it like this so people could follow closely as I pushed the > locking down. Most of these "push extent lock into.." patches do a variation of > this, where I push it down into a function and move the lock down in the set of > things. I'm happy to merge them together, but I split it this way so my logic > could be followed. The incremental changes are better for review, moving locks affects code before and after and it's more convienient to review each step as the amount of information and context is bearable. So I'm fine with the way you did it.
On 6:33 23/04, Goldwyn Rodrigues wrote: > On 10:35 17/04, Josef Bacik wrote: > > run_delalloc_nocow is a bit special as it walks through the file extents > > for the inode and determines what it can nocow and what it can't. This > > is the more complicated area for extent locking, so start with this > > function. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > > --- > > fs/btrfs/inode.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 2083005f2828..f14b3cecce47 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, > > */ > > ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root)); > > > > + lock_extent(&inode->io_tree, start, end, NULL); > > + > > path = btrfs_alloc_path(); > > if (!path) { > > ret = -ENOMEM; > > @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page > > const bool zoned = btrfs_is_zoned(inode->root->fs_info); > > int ret; > > > > - /* > > - * We're unlocked by the different fill functions below. > > - */ > > - lock_extent(&inode->io_tree, start, end, NULL); > > - > > So, you are adding this hunk in the previous patch and (re)moving it here. > Do you think it would be better to merge this with the previous patch? Ignore this comment. This patch series is doing just that and I should have commented after looking at the series as a whole. Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 2083005f2828..f14b3cecce47 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode, */ ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root)); + lock_extent(&inode->io_tree, start, end, NULL); + path = btrfs_alloc_path(); if (!path) { ret = -ENOMEM; @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page const bool zoned = btrfs_is_zoned(inode->root->fs_info); int ret; - /* - * We're unlocked by the different fill functions below. - */ - lock_extent(&inode->io_tree, start, end, NULL); - /* * The range must cover part of the @locked_page, or a return of 1 * can confuse the caller. @@ -2266,6 +2263,11 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page goto out; } + /* + * We're unlocked by the different fill functions below. + */ + lock_extent(&inode->io_tree, start, end, NULL); + if (btrfs_inode_can_compress(inode) && inode_need_compress(inode, start, end) && run_delalloc_compressed(inode, locked_page, start, end, wbc))
run_delalloc_nocow is a bit special as it walks through the file extents for the inode and determines what it can nocow and what it can't. This is the more complicated area for extent locking, so start with this function. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/inode.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)