| Message ID | 20201103133108.148112-9-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/32] btrfs: extent_io: remove the extent_start/extent_len for end_bio_extent_readpage() | expand |
On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: > For __set_extent_bit(), those parameter are less common for most > callers: > - exclusive_bits > - failed_start > Mostly for extent locking. > > - extent_changeset > For qgroup usage. > > As a common design principle, less common parameters should have their > default values and only callers really need them will set the parameters > to non-default values. > > Sink those parameters into a new structure, extent_io_extra_options. > So most callers won't bother those less used parameters, and make later > expansion easier. > > Signed-off-by: Qu Wenruo <wqu@suse.com> IMO I feel this is an overkill, __set_extent_Bit is really some low-level, hidden interface which is being wrapped around by other high-level extent bit manipulation functions. Following this logic I did send today a patch renaming __set_extent_bit to set_extent_bit, essentially removing a level of indirection. Having said that what you are doing right now might make more sense for future changes since you state it's preparatory anyway. But in any case I believe the interface for this function is just broken if we have to resort to such type of refactoring. See below for one idea of extent bits handling. > --- > fs/btrfs/extent-io-tree.h | 22 ++++++++++++++ > fs/btrfs/extent_io.c | 61 ++++++++++++++++++++++++--------------- > 2 files changed, 59 insertions(+), 24 deletions(-) > > diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h > index cab4273ff8d3..c93065794567 100644 > --- a/fs/btrfs/extent-io-tree.h > +++ b/fs/btrfs/extent-io-tree.h > @@ -82,6 +82,28 @@ struct extent_state { > #endif > }; > > +/* > + * Extra options for extent io tree operations. > + * > + * All of these options are initialized to 0/false/NULL by default, > + * and most callers should utilize the wrappers other than the extra options. > + */ > +struct extent_io_extra_options { > + /* > + * For __set_extent_bit(), to return -EEXIST when hit an extent with > + * @excl_bits set, and update @excl_failed_start. > + * Utizlied by EXTENT_LOCKED wrappers. nit: excl_bits can be removed if we simply check for the presence of EXTENT_LOCKED in 'bits' and if the result is true then also check if the found extent has EXTENT_LOCKED if it does -> return the failure_start, that we we can get rid of 'excl_bits'. All uses of EXTENT_LOCKED is via lock_extent_range except the one in the private failure tree but I believe it should be ok. I had a crazy idea to overload cached_state to return the failure range and use it in lock_extent_bit but that makes it rather messy. ... > + */ > + u32 excl_bits; > + u64 excl_failed_start; > + > + /* > + * For __set/__clear_extent_bit() to record how many bytes is modified. > + * For qgroup related functions. > + */ > + struct extent_changeset *changeset; > +}; > + > int __init extent_state_cache_init(void); > void __cold extent_state_cache_exit(void); > <snip>
On 2020/11/5 下午9:35, Nikolay Borisov wrote: > > > On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: >> For __set_extent_bit(), those parameter are less common for most >> callers: >> - exclusive_bits >> - failed_start >> Mostly for extent locking. >> >> - extent_changeset >> For qgroup usage. >> >> As a common design principle, less common parameters should have their >> default values and only callers really need them will set the parameters >> to non-default values. >> >> Sink those parameters into a new structure, extent_io_extra_options. >> So most callers won't bother those less used parameters, and make later >> expansion easier. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > IMO I feel this is an overkill, __set_extent_Bit is really some > low-level, hidden interface which is being wrapped around by other > high-level extent bit manipulation functions. Following this logic I did > send today a patch renaming __set_extent_bit to set_extent_bit, > essentially removing a level of indirection. > > Having said that what you are doing right now might make more sense for > future changes since you state it's preparatory anyway. But in any case > I believe the interface for this function is just broken if we have to > resort to such type of refactoring. > > See below for one idea of extent bits handling. > >> --- >> fs/btrfs/extent-io-tree.h | 22 ++++++++++++++ >> fs/btrfs/extent_io.c | 61 ++++++++++++++++++++++++--------------- >> 2 files changed, 59 insertions(+), 24 deletions(-) >> >> diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h >> index cab4273ff8d3..c93065794567 100644 >> --- a/fs/btrfs/extent-io-tree.h >> +++ b/fs/btrfs/extent-io-tree.h >> @@ -82,6 +82,28 @@ struct extent_state { >> #endif >> }; >> >> +/* >> + * Extra options for extent io tree operations. >> + * >> + * All of these options are initialized to 0/false/NULL by default, >> + * and most callers should utilize the wrappers other than the extra options. >> + */ >> +struct extent_io_extra_options { >> + /* >> + * For __set_extent_bit(), to return -EEXIST when hit an extent with >> + * @excl_bits set, and update @excl_failed_start. >> + * Utizlied by EXTENT_LOCKED wrappers. > > nit: excl_bits can be removed if we simply check for the presence of > EXTENT_LOCKED in 'bits' and if the result is true then also check if the > found extent has EXTENT_LOCKED if it does -> return the failure_start, > that we we can get rid of 'excl_bits'. All uses of EXTENT_LOCKED is via > lock_extent_range except the one in the private failure tree but I > believe it should be ok. Nope, there are users for excl_bits without using EXTENT_LOCKED. It's from compression code, although I doubt if the existing code is correct, but at least I kept the behavior the same for right now. And you will see more parameters moved to this structure in the rest of the series. Like wake/delete, skip_lock and prealloc, where the last two are mostly for subpage only cases. With all these extra options involved, I hope the need is more obvious. Thanks, Qu > > I had a crazy idea to overload cached_state to return the failure range > and use it in lock_extent_bit but that makes it rather messy. ... > >> + */ >> + u32 excl_bits; >> + u64 excl_failed_start; >> + >> + /* >> + * For __set/__clear_extent_bit() to record how many bytes is modified. >> + * For qgroup related functions. >> + */ >> + struct extent_changeset *changeset; >> +}; >> + >> int __init extent_state_cache_init(void); >> void __cold extent_state_cache_exit(void); >> > > <snip> > >
diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h index cab4273ff8d3..c93065794567 100644 --- a/fs/btrfs/extent-io-tree.h +++ b/fs/btrfs/extent-io-tree.h @@ -82,6 +82,28 @@ struct extent_state { #endif }; +/* + * Extra options for extent io tree operations. + * + * All of these options are initialized to 0/false/NULL by default, + * and most callers should utilize the wrappers other than the extra options. + */ +struct extent_io_extra_options { + /* + * For __set_extent_bit(), to return -EEXIST when hit an extent with + * @excl_bits set, and update @excl_failed_start. + * Utizlied by EXTENT_LOCKED wrappers. + */ + u32 excl_bits; + u64 excl_failed_start; + + /* + * For __set/__clear_extent_bit() to record how many bytes is modified. + * For qgroup related functions. + */ + struct extent_changeset *changeset; +}; + int __init extent_state_cache_init(void); void __cold extent_state_cache_exit(void); diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index a90cdcf01b7f..1fd92815553d 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -29,6 +29,7 @@ static struct kmem_cache *extent_state_cache; static struct kmem_cache *extent_buffer_cache; static struct bio_set btrfs_bioset; +static struct extent_io_extra_options default_opts = { 0 }; static inline bool extent_state_in_tree(const struct extent_state *state) { return !RB_EMPTY_NODE(&state->rb_node); @@ -952,10 +953,10 @@ static void cache_state(struct extent_state *state, } /* - * set some bits on a range in the tree. This may require allocations or + * Set some bits on a range in the tree. This may require allocations or * sleeping, so the gfp mask is used to indicate what is allowed. * - * If any of the exclusive bits are set, this will fail with -EEXIST if some + * If *any* of the exclusive bits are set, this will fail with -EEXIST if some * part of the range already has the desired bits set. The start of the * existing range is returned in failed_start in this case. * @@ -964,26 +965,30 @@ static void cache_state(struct extent_state *state, static int __must_check __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end, - unsigned bits, unsigned exclusive_bits, - u64 *failed_start, struct extent_state **cached_state, - gfp_t mask, struct extent_changeset *changeset) + unsigned bits, struct extent_state **cached_state, + gfp_t mask, struct extent_io_extra_options *extra_opts) { struct extent_state *state; struct extent_state *prealloc = NULL; struct rb_node *node; struct rb_node **p; struct rb_node *parent; + struct extent_changeset *changeset; int err = 0; + u32 exclusive_bits; + u64 *failed_start; u64 last_start; u64 last_end; btrfs_debug_check_extent_io_range(tree, start, end); trace_btrfs_set_extent_bit(tree, start, end - start + 1, bits); - if (exclusive_bits) - ASSERT(failed_start); - else - ASSERT(failed_start == NULL); + if (!extra_opts) + extra_opts = &default_opts; + exclusive_bits = extra_opts->excl_bits; + failed_start = &extra_opts->excl_failed_start; + changeset = extra_opts->changeset; + again: if (!prealloc && gfpflags_allow_blocking(mask)) { /* @@ -1186,7 +1191,7 @@ __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end, int set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end, unsigned bits, struct extent_state **cached_state, gfp_t mask) { - return __set_extent_bit(tree, start, end, bits, 0, NULL, cached_state, + return __set_extent_bit(tree, start, end, bits, cached_state, mask, NULL); } @@ -1413,6 +1418,10 @@ int convert_extent_bit(struct extent_io_tree *tree, u64 start, u64 end, int set_record_extent_bits(struct extent_io_tree *tree, u64 start, u64 end, unsigned bits, struct extent_changeset *changeset) { + struct extent_io_extra_options extra_opts = { + .changeset = changeset, + }; + /* * We don't support EXTENT_LOCKED yet, as current changeset will * record any bits changed, so for EXTENT_LOCKED case, it will @@ -1421,15 +1430,14 @@ int set_record_extent_bits(struct extent_io_tree *tree, u64 start, u64 end, */ BUG_ON(bits & EXTENT_LOCKED); - return __set_extent_bit(tree, start, end, bits, 0, NULL, NULL, GFP_NOFS, - changeset); + return __set_extent_bit(tree, start, end, bits, NULL, GFP_NOFS, + &extra_opts); } int set_extent_bits_nowait(struct extent_io_tree *tree, u64 start, u64 end, unsigned bits) { - return __set_extent_bit(tree, start, end, bits, 0, NULL, NULL, - GFP_NOWAIT, NULL); + return __set_extent_bit(tree, start, end, bits, NULL, GFP_NOWAIT, NULL); } int clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end, @@ -1460,16 +1468,18 @@ int clear_record_extent_bits(struct extent_io_tree *tree, u64 start, u64 end, int lock_extent_bits(struct extent_io_tree *tree, u64 start, u64 end, struct extent_state **cached_state) { + struct extent_io_extra_options extra_opts = { + .excl_bits = EXTENT_LOCKED, + }; int err; - u64 failed_start; while (1) { err = __set_extent_bit(tree, start, end, EXTENT_LOCKED, - EXTENT_LOCKED, &failed_start, - cached_state, GFP_NOFS, NULL); + cached_state, GFP_NOFS, &extra_opts); if (err == -EEXIST) { - wait_extent_bit(tree, failed_start, end, EXTENT_LOCKED); - start = failed_start; + wait_extent_bit(tree, extra_opts.excl_failed_start, end, + EXTENT_LOCKED); + start = extra_opts.excl_failed_start; } else break; WARN_ON(start > end); @@ -1479,14 +1489,17 @@ int lock_extent_bits(struct extent_io_tree *tree, u64 start, u64 end, int try_lock_extent(struct extent_io_tree *tree, u64 start, u64 end) { + struct extent_io_extra_options extra_opts = { + .excl_bits = EXTENT_LOCKED, + }; int err; - u64 failed_start; - err = __set_extent_bit(tree, start, end, EXTENT_LOCKED, EXTENT_LOCKED, - &failed_start, NULL, GFP_NOFS, NULL); + err = __set_extent_bit(tree, start, end, EXTENT_LOCKED, + NULL, GFP_NOFS, &extra_opts); if (err == -EEXIST) { - if (failed_start > start) - clear_extent_bit(tree, start, failed_start - 1, + if (extra_opts.excl_failed_start > start) + clear_extent_bit(tree, start, + extra_opts.excl_failed_start - 1, EXTENT_LOCKED, 1, 0, NULL); return 0; }
For __set_extent_bit(), those parameter are less common for most callers: - exclusive_bits - failed_start Mostly for extent locking. - extent_changeset For qgroup usage. As a common design principle, less common parameters should have their default values and only callers really need them will set the parameters to non-default values. Sink those parameters into a new structure, extent_io_extra_options. So most callers won't bother those less used parameters, and make later expansion easier. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent-io-tree.h | 22 ++++++++++++++ fs/btrfs/extent_io.c | 61 ++++++++++++++++++++++++--------------- 2 files changed, 59 insertions(+), 24 deletions(-)