From: Yu Kuai <yukuai3@huawei.com>

If {start, end}_discard is implemented from bitmap_operations, then they
will be used to handle discard IO. Currently md-bitmap handle discard
the same as normal write, prepare to support discard for new md bitmap.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 19 +++++++++++++++----
 drivers/md/md.h |  1 +
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 32b997dfe6f4..c72c13ed4253 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8849,14 +8849,24 @@ static void md_bitmap_start(struct mddev *mddev,
 		mddev->pers->bitmap_sector(mddev, &md_io_clone->offset,
 					   &md_io_clone->sectors);
 
-	mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
-				      md_io_clone->sectors);
+	if (unlikely(md_io_clone->rw == STAT_DISCARD) &&
+	    mddev->bitmap_ops->start_discard)
+		mddev->bitmap_ops->start_discard(mddev, md_io_clone->offset,
+						 md_io_clone->sectors);
+	else
+		mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
+					      md_io_clone->sectors);
 }
 
 static void md_bitmap_end(struct mddev *mddev, struct md_io_clone *md_io_clone)
 {
-	mddev->bitmap_ops->endwrite(mddev, md_io_clone->offset,
-				    md_io_clone->sectors);
+	if (unlikely(md_io_clone->rw == STAT_DISCARD) &&
+	    mddev->bitmap_ops->end_discard)
+		mddev->bitmap_ops->end_discard(mddev, md_io_clone->offset,
+					       md_io_clone->sectors);
+	else
+		mddev->bitmap_ops->endwrite(mddev, md_io_clone->offset,
+					    md_io_clone->sectors);
 }
 
 static void md_end_clone_io(struct bio *bio)
@@ -8895,6 +8905,7 @@ static void md_clone_bio(struct mddev *mddev, struct bio **bio)
 	if (bio_data_dir(*bio) == WRITE && md_bitmap_enabled(mddev)) {
 		md_io_clone->offset = (*bio)->bi_iter.bi_sector;
 		md_io_clone->sectors = bio_sectors(*bio);
+		md_io_clone->rw = op_stat_group(bio_op(*bio));
 		md_bitmap_start(mddev, md_io_clone);
 	}
 
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 6eb5dfdf2f55..c474bf74c345 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -850,6 +850,7 @@ struct md_io_clone {
 	unsigned long	start_time;
 	sector_t	offset;
 	unsigned long	sectors;
+	enum stat_group	rw;
 	struct bio	bio_clone;
 };
 
-- 
2.39.2

On Mon, May 12, 2025 at 09:19:10AM +0800, Yu Kuai wrote:
> +++ b/drivers/md/md.c
> @@ -8849,14 +8849,24 @@ static void md_bitmap_start(struct mddev *mddev,
>  		mddev->pers->bitmap_sector(mddev, &md_io_clone->offset,
>  					   &md_io_clone->sectors);
>  
> -	mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
> -				      md_io_clone->sectors);
> +	if (unlikely(md_io_clone->rw == STAT_DISCARD) &&
> +	    mddev->bitmap_ops->start_discard)
> +		mddev->bitmap_ops->start_discard(mddev, md_io_clone->offset,
> +						 md_io_clone->sectors);
> +	else
> +		mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
> +					      md_io_clone->sectors);
>  }

This interface feels weird, as it would still call into the write
interfaces when the discard ones are not defined instead of doing
nothing.  Also shouldn't discard also use a different interface
than md_bitmap_start in the caller?

I'd also expect the final version of this to be merged with the
previous patch, as adding an interface without the only user is a
bit odd.

Hi,

在 2025/05/12 12:41, Christoph Hellwig 写道:
> On Mon, May 12, 2025 at 09:19:10AM +0800, Yu Kuai wrote:
>> +++ b/drivers/md/md.c
>> @@ -8849,14 +8849,24 @@ static void md_bitmap_start(struct mddev *mddev,
>>   		mddev->pers->bitmap_sector(mddev, &md_io_clone->offset,
>>   					   &md_io_clone->sectors);
>>   
>> -	mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
>> -				      md_io_clone->sectors);
>> +	if (unlikely(md_io_clone->rw == STAT_DISCARD) &&
>> +	    mddev->bitmap_ops->start_discard)
>> +		mddev->bitmap_ops->start_discard(mddev, md_io_clone->offset,
>> +						 md_io_clone->sectors);
>> +	else
>> +		mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
>> +					      md_io_clone->sectors);
>>   }
> 
> This interface feels weird, as it would still call into the write
> interfaces when the discard ones are not defined instead of doing
> nothing.  Also shouldn't discard also use a different interface
> than md_bitmap_start in the caller?

This is because the old bitmap handle discard the same as write, I
can't do nothing in this case. Do you prefer also reuse the write
api to new discard api for old bitmap?

For the caller, I think it's fine, since bitmap framwork already
calculate sectors and len for discard as well.
> 
> I'd also expect the final version of this to be merged with the
> previous patch, as adding an interface without the only user is a
> bit odd.

Sure.

Thanks,
Kuai

> .
> 

On Mon, May 12, 2025 at 02:05:56PM +0800, Yu Kuai wrote:
>>>   -	mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
>>> -				      md_io_clone->sectors);
>>> +	if (unlikely(md_io_clone->rw == STAT_DISCARD) &&
>>> +	    mddev->bitmap_ops->start_discard)
>>> +		mddev->bitmap_ops->start_discard(mddev, md_io_clone->offset,
>>> +						 md_io_clone->sectors);
>>> +	else
>>> +		mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
>>> +					      md_io_clone->sectors);
>>>   }
>>
>> This interface feels weird, as it would still call into the write
>> interfaces when the discard ones are not defined instead of doing
>> nothing.  Also shouldn't discard also use a different interface
>> than md_bitmap_start in the caller?
>
> This is because the old bitmap handle discard the same as write, I
> can't do nothing in this case. Do you prefer also reuse the write
> api to new discard api for old bitmap?

It can just point the discard method to the same function as the
existing write one.

Hi,

在 2025/05/12 14:12, Christoph Hellwig 写道:
> On Mon, May 12, 2025 at 02:05:56PM +0800, Yu Kuai wrote:
>>>>    -	mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
>>>> -				      md_io_clone->sectors);
>>>> +	if (unlikely(md_io_clone->rw == STAT_DISCARD) &&
>>>> +	    mddev->bitmap_ops->start_discard)
>>>> +		mddev->bitmap_ops->start_discard(mddev, md_io_clone->offset,
>>>> +						 md_io_clone->sectors);
>>>> +	else
>>>> +		mddev->bitmap_ops->startwrite(mddev, md_io_clone->offset,
>>>> +					      md_io_clone->sectors);
>>>>    }
>>>
>>> This interface feels weird, as it would still call into the write
>>> interfaces when the discard ones are not defined instead of doing
>>> nothing.  Also shouldn't discard also use a different interface
>>> than md_bitmap_start in the caller?
>>
>> This is because the old bitmap handle discard the same as write, I
>> can't do nothing in this case. Do you prefer also reuse the write
>> api to new discard api for old bitmap?
> 
> It can just point the discard method to the same function as the
> existing write one.

Yes, this is exactly want I mean, I'll update this in the next version.

Thanks,
Kuai

> 
> .
>