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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250512051519.GA1555@lst.de>
Date: Mon, 12 May 2025 07:15:19 +0200
From: Christoph Hellwig <hch@....de>
To: Yu Kuai <yukuai1@...weicloud.com>
Cc: hch@....de, xni@...hat.com, colyli@...nel.org, agk@...hat.com,
	snitzer@...nel.org, mpatocka@...hat.com, song@...nel.org,
	yukuai3@...wei.com, linux-kernel@...r.kernel.org,
	dm-devel@...ts.linux.dev, linux-raid@...r.kernel.org,
	yi.zhang@...wei.com, yangerkun@...wei.com, johnny.chenyi@...wei.com
Subject: Re: [PATCH RFC md-6.16 v3 11/19] md/md-llbitmap: implement bitmap
 IO

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..

> +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.

> +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?

> +	/*
> +	 * 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?

> +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.

> +		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.

> +	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?