diff mbox series

[v4,04/28] range: Add range_overlaps()

Message ID 20241007-dcd-type2-upstream-v4-4-c261ee6eeded@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ira Weiny
Headers show
Series DCD: Add support for Dynamic Capacity Devices (DCD) | expand

Commit Message

Ira Weiny Oct. 7, 2024, 11:16 p.m. UTC 
Code to support CXL Dynamic Capacity devices will have extent ranges
which need to be compared for intersection not a subset as is being
checked in range_contains().

range_overlaps() is defined in btrfs with a different meaning from what
is required in the standard range code.  Dan Williams pointed this out
in [1].  Adjust the btrfs call according to his suggestion there.

Then add a generic range_overlaps().

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org

Acked-by: David Sterba <dsterba@suse.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

[1] https://lore.kernel.org/all/65949f79ef908_8dc68294f2@dwillia2-xfh.jf.intel.com.notmuch/
---
 fs/btrfs/ordered-data.c | 10 +++++-----
 include/linux/range.h   |  7 +++++++
 2 files changed, 12 insertions(+), 5 deletions(-)

Comments

David Sterba Oct. 8, 2024, 4:10 p.m. UTC | #1 
On Mon, Oct 07, 2024 at 06:16:10PM -0500, Ira Weiny wrote:
> --- a/include/linux/range.h
> +++ b/include/linux/range.h
> +/* True if any part of r1 overlaps r2 */
> +static inline bool range_overlaps(struct range *r1, struct range *r2)

I've noticed only now, you can constify the arguments, but this applise
to other range_* functions so that can be done later in one go.

> +{
> +	return r1->start <= r2->end && r1->end >= r2->start;
> +}
Andy Shevchenko Oct. 9, 2024, 2:45 p.m. UTC | #2 
On Tue, Oct 08, 2024 at 06:10:32PM +0200, David Sterba wrote:
> On Mon, Oct 07, 2024 at 06:16:10PM -0500, Ira Weiny wrote:

...

> > +static inline bool range_overlaps(struct range *r1, struct range *r2)
> 
> I've noticed only now, you can constify the arguments, but this applise
> to other range_* functions so that can be done later in one go.

Frankly you may add the same to each new API being added to the file and
the "one go" will never happen. So, I support your first part with
constifying, but I think it would be rather done now to start that "one
go" to happen.
Andy Shevchenko Oct. 9, 2024, 2:46 p.m. UTC | #3 
On Wed, Oct 09, 2024 at 05:45:10PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 08, 2024 at 06:10:32PM +0200, David Sterba wrote:
> > On Mon, Oct 07, 2024 at 06:16:10PM -0500, Ira Weiny wrote:

...

> > > +static inline bool range_overlaps(struct range *r1, struct range *r2)
> > 
> > I've noticed only now, you can constify the arguments, but this applise
> > to other range_* functions so that can be done later in one go.
> 
> Frankly you may add the same to each new API being added to the file and
> the "one go" will never happen. So, I support your first part with
> constifying, but I think it would be rather done now to start that "one
> go" to happen.

Alternatively there is should be the patch _in this series_ to make it
happen before extending an API. I leave the choice to Ira.
David Sterba Oct. 9, 2024, 3:36 p.m. UTC | #4 
On Wed, Oct 09, 2024 at 05:45:10PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 08, 2024 at 06:10:32PM +0200, David Sterba wrote:
> > On Mon, Oct 07, 2024 at 06:16:10PM -0500, Ira Weiny wrote:
> 
> ...
> 
> > > +static inline bool range_overlaps(struct range *r1, struct range *r2)
> > 
> > I've noticed only now, you can constify the arguments, but this applise
> > to other range_* functions so that can be done later in one go.
> 
> Frankly you may add the same to each new API being added to the file and
> the "one go" will never happen.

Yeah, but it's a minor issue for a 28 patchset, I don't know if there
are some other major things still to do so that a v5 is expected.

If anybody is interested, reviewing APIs and interfaces with focus on
some data structure and const is relatively easy, compile test is
typically enough. The hard part is to find the missing ones. There's no
compiler aid thad I'd know of (-Wsuggest-attribute=const is not for
parameters), so it's been reading a file top-down for me.

> So, I support your first part with
> constifying, but I think it would be rather done now to start that "one
> go" to happen.

Agreed, one patch on top is probably the least intrusive way.
Andy Shevchenko Oct. 9, 2024, 4:04 p.m. UTC | #5 
On Wed, Oct 09, 2024 at 05:36:42PM +0200, David Sterba wrote:
> On Wed, Oct 09, 2024 at 05:45:10PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 08, 2024 at 06:10:32PM +0200, David Sterba wrote:
> > > On Mon, Oct 07, 2024 at 06:16:10PM -0500, Ira Weiny wrote:

...

> > > > +static inline bool range_overlaps(struct range *r1, struct range *r2)
> > > 
> > > I've noticed only now, you can constify the arguments, but this applise
> > > to other range_* functions so that can be done later in one go.
> > 
> > Frankly you may add the same to each new API being added to the file and
> > the "one go" will never happen.
> 
> Yeah, but it's a minor issue for a 28 patchset, I don't know if there
> are some other major things still to do so that a v5 is expected.

At least seems printf() changes have to be amended, so I think v5 is
warranted anyway.

> If anybody is interested, reviewing APIs and interfaces with focus on
> some data structure and const is relatively easy, compile test is
> typically enough.

Except the cases where a const pointer has to be passed thru non-const
(or integer) field in a data structure. Tons of the existing examples is
ID tables that wanted to have kernel_ulong_t instead of const void * in
driver data field.

> The hard part is to find the missing ones. There's no
> compiler aid thad I'd know of (-Wsuggest-attribute=const is not for
> parameters), so it's been reading a file top-down for me.

Yeah...

> > So, I support your first part with
> > constifying, but I think it would be rather done now to start that "one
> > go" to happen.
> 
> Agreed, one patch on top is probably the least intrusive way.
Ira Weiny Oct. 10, 2024, 3:24 p.m. UTC | #6 
David Sterba wrote:
> On Mon, Oct 07, 2024 at 06:16:10PM -0500, Ira Weiny wrote:
> > --- a/include/linux/range.h
>> +++ b/include/linux/range.h
> > +/* True if any part of r1 overlaps r2 */
> > +static inline bool range_overlaps(struct range *r1, struct range *r2)
> 
> I've noticed only now, you can constify the arguments, but this applise
> to other range_* functions so that can be done later in one go.

Looks like there will be a v5.  I'll do a separate cleanup patch for
range_contains() and change this one.

Thanks!
Ira

> 
> > +{
> > +	return r1->start <= r2->end && r1->end >= r2->start;
> > +}
Ira Weiny Oct. 14, 2024, 12:12 a.m. UTC | #7 
Andy Shevchenko wrote:
> On Wed, Oct 09, 2024 at 05:45:10PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 08, 2024 at 06:10:32PM +0200, David Sterba wrote:
> > > On Mon, Oct 07, 2024 at 06:16:10PM -0500, Ira Weiny wrote:
> 
> ...
> 
> > > > +static inline bool range_overlaps(struct range *r1, struct range *r2)
> > > 
> > > I've noticed only now, you can constify the arguments, but this applise
> > > to other range_* functions so that can be done later in one go.
> > 
> > Frankly you may add the same to each new API being added to the file and
> > the "one go" will never happen. So, I support your first part with
> > constifying, but I think it would be rather done now to start that "one
> > go" to happen.
> 
> Alternatively there is should be the patch _in this series_ to make it
> happen before extending an API. I leave the choice to Ira.

I'm not sure I follow what you are saying here but I think you are saying to
make those calls const prior to adding new ones.

I agree see:

https://lore.kernel.org/all/20241010-const-range-v1-1-afb6e4bfd8ce@intel.com/

Hopefully this is what you meant and closes this issue.
Ira
diff mbox series

Patch

diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index 2104d60c2161..744c3375ee6a 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -111,8 +111,8 @@  static struct rb_node *__tree_search(struct rb_root *root, u64 file_offset,
 	return NULL;
 }
 
-static int range_overlaps(struct btrfs_ordered_extent *entry, u64 file_offset,
-			  u64 len)
+static int btrfs_range_overlaps(struct btrfs_ordered_extent *entry, u64 file_offset,
+				u64 len)
 {
 	if (file_offset + len <= entry->file_offset ||
 	    entry->file_offset + entry->num_bytes <= file_offset)
@@ -985,7 +985,7 @@  struct btrfs_ordered_extent *btrfs_lookup_ordered_range(
 
 	while (1) {
 		entry = rb_entry(node, struct btrfs_ordered_extent, rb_node);
-		if (range_overlaps(entry, file_offset, len))
+		if (btrfs_range_overlaps(entry, file_offset, len))
 			break;
 
 		if (entry->file_offset >= file_offset + len) {
@@ -1114,12 +1114,12 @@  struct btrfs_ordered_extent *btrfs_lookup_first_ordered_range(
 	}
 	if (prev) {
 		entry = rb_entry(prev, struct btrfs_ordered_extent, rb_node);
-		if (range_overlaps(entry, file_offset, len))
+		if (btrfs_range_overlaps(entry, file_offset, len))
 			goto out;
 	}
 	if (next) {
 		entry = rb_entry(next, struct btrfs_ordered_extent, rb_node);
-		if (range_overlaps(entry, file_offset, len))
+		if (btrfs_range_overlaps(entry, file_offset, len))
 			goto out;
 	}
 	/* No ordered extent in the range */
diff --git a/include/linux/range.h b/include/linux/range.h
index 6ad0b73cb7ad..9a46f3212965 100644
--- a/include/linux/range.h
+++ b/include/linux/range.h
@@ -13,11 +13,18 @@  static inline u64 range_len(const struct range *range)
 	return range->end - range->start + 1;
 }
 
+/* True if r1 completely contains r2 */
 static inline bool range_contains(struct range *r1, struct range *r2)
 {
 	return r1->start <= r2->start && r1->end >= r2->end;
 }
 
+/* True if any part of r1 overlaps r2 */
+static inline bool range_overlaps(struct range *r1, struct range *r2)
+{
+	return r1->start <= r2->end && r1->end >= r2->start;
+}
+
 int add_range(struct range *range, int az, int nr_range,
 		u64 start, u64 end);