Re: [PATCH RFC md-6.16 v3 11/19] md/md-llbitmap: implement bitmap IO

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/05/12 13:15, Christoph Hellwig 写道:
> On Mon, May 12, 2025 at 09:19:19AM +0800, Yu Kuai wrote:
>> +static bool is_raid456(struct mddev *mddev)
>> +{
>> +	return (mddev->level == 4 || mddev->level == 5 || mddev->level == 6);
>> +}
> 
> This really should be in a common helper somewhere..

Perhaps md.h?
> 
>> +static int llbitmap_read(struct llbitmap *llbitmap, enum llbitmap_state *state,
>> +			 loff_t pos)
>> +{
>> +	pos += BITMAP_SB_SIZE;
>> +	*state = llbitmap->barrier[pos >> PAGE_SHIFT].data[offset_in_page(pos)];
>> +	return 0;
>> +}
> 
> This always return 0, and could just return void.

Ok
> 
>> +static void llbitmap_set_page_dirty(struct llbitmap *llbitmap, int idx, int offset)
> 
> Overly long line.
> 
> Also should the second and third argument be unsigned?

Ok
> 
>> +	/*
>> +	 * if the bit is already dirty, or other page bytes is the same bit is
>> +	 * already BitDirty, then mark the whole bytes in the bit as dirty
>> +	 */
>> +	if (test_and_set_bit(bit, barrier->dirty)) {
>> +		infectious = true;
>> +	} else {
>> +		for (pos = bit * io_size; pos < (bit + 1) * io_size - 1;
>> +		     pos++) {
>> +			if (pos == offset)
>> +				continue;
>> +			if (barrier->data[pos] == BitDirty ||
>> +			    barrier->data[pos] == BitNeedSync) {
>> +				infectious = true;
>> +				break;
>> +			}
>> +		}
>> +
>> +	}
>> +	if (!infectious)
>> +		return;
> 
> Mabe use a goto and/or a helper function containing the for loop to
> clean up the control flow here a bit?

Ok, I;ll add a helper function.
> 
>> +static int llbitmap_write(struct llbitmap *llbitmap, enum llbitmap_state state,
>> +			  loff_t pos)
>> +{
>> +	int idx;
>> +	int offset;
> 
> Unsigned?
> 
>> +
>> +	pos += BITMAP_SB_SIZE;
>> +	idx = pos >> PAGE_SHIFT;
>> +	offset = offset_in_page(pos);
>> +
>> +	llbitmap->barrier[idx].data[offset] = state;
>> +	if (state == BitDirty || state == BitNeedSync)
>> +		llbitmap_set_page_dirty(llbitmap, idx, offset);
>> +	return 0;
> 
> and this could also be a void return.

Ok.
> 
>> +		sector = mddev->bitmap_info.offset + (idx << PAGE_SECTORS_SHIFT);
> 
> Overly long line.
> 
>> +			if (rdev->raid_disk < 0 || test_bit(Faulty, &rdev->flags))
> 
> Same here.
> 
>> +	int nr_pages = (llbitmap->chunks + BITMAP_SB_SIZE + PAGE_SIZE - 1) / PAGE_SIZE;
> 
> Unsigned for the type, and DIV_ROUND_UP for the calculation.

Ok.
> 
>> +	struct page *page;
>> +	int i = 0;
>> +
>> +	llbitmap->nr_pages = nr_pages;
>> +	while (i < nr_pages) {
>> +		page = llbitmap_read_page(llbitmap, i);
>> +		if (IS_ERR(page)) {
>> +			llbitmap_free_pages(llbitmap);
>> +			return PTR_ERR(page);
>> +		}
>> +
>> +		if (percpu_ref_init(&llbitmap->barrier[i].active, active_release,
>> +				    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
>> +			__free_page(page);
>> +			return -ENOMEM;
> 
> Doesn't this also need a llbitmap_free_pages for the error case?

Of course, my bad that I forgot this.

Thanks again for the review!
Kuai

> 
> .
>