From: "heming.zhao@suse.com" <heming.zhao@suse.com>
To: Guoqing Jiang <guoqing.jiang@linux.dev>,
Kent Overstreet <kent.overstreet@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org, linux-raid@vger.kernel.org,
linux-block@vger.kernel.org, axboe@kernel.dk,
alexander.h.duyck@linux.intel.com
Subject: Re: [PATCH v2] md: Kill usage of page->index
Date: Fri, 15 Oct 2021 16:59:11 +0800 [thread overview]
Message-ID: <2c33ebaf-66d2-3138-9e75-ab366b395ad9@suse.com> (raw)
In-Reply-To: <c2e2edd1-8f6f-1849-df0a-46916e311586@linux.dev>
I agree Guoqing's comments.
In Documentation/driver-api/md/md-cluster.rst, there is a pic in "1. On-disk format".
Which describes the layout of "sb+bitmap" area.
And even in non-cluster array, bitmap area more than 1 pages also needs index/offset.
After the serial patches removing page->index, md layer should create a similar
struct to manage it.
- Heming
On 10/15/21 11:01, Guoqing Jiang wrote:
>
>
> On 10/14/21 10:30 PM, Kent Overstreet wrote:
>> On Thu, Oct 14, 2021 at 04:58:46PM +0800,heming.zhao@suse.com wrote:
>>> Hello all,
>>>
>>> The page->index takes an important role for cluster-md module.
>>> i.e, a two-node HA env, node A bitmap may be managed in first 4K area, then
>>> node B bitmap is in 8K area (the second page). this patch removes the index
>>> and fix/hardcode index with value 0, which will only operate first node bitmap.
>>>
>>> If this serial patches are important and must be merged in mainline, we should
>>> redesign code logic for the related code.
>>>
>>> Thanks,
>>> Heming
>> Can you look at and test the updated patch below? The more I look at the md
>> bitmap code the more it scares me.
>>
>> -- >8 --
>> Subject: [PATCH] md: Kill usage of page->index
>>
>> As part of the struct page cleanups underway, we want to remove as much
>> usage of page->mapping and page->index as possible, as frequently they
>> are known from context - as they are here in the md bitmap code.
>>
>> Signed-off-by: Kent Overstreet<kent.overstreet@gmail.com>
>> ---
>> drivers/md/md-bitmap.c | 49 ++++++++++++++++++++----------------------
>> drivers/md/md-bitmap.h | 1 +
>> 2 files changed, 24 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>> index e29c6298ef..316e4cd5a7 100644
>> --- a/drivers/md/md-bitmap.c
>> +++ b/drivers/md/md-bitmap.c
>> @@ -165,10 +165,8 @@ static int read_sb_page(struct mddev *mddev, loff_t offset,
>> if (sync_page_io(rdev, target,
>> roundup(size, bdev_logical_block_size(rdev->bdev)),
>> - page, REQ_OP_READ, 0, true)) {
>> - page->index = index;
>> + page, REQ_OP_READ, 0, true))
>> return 0;
>> - }
>> }
>> return -EIO;
>> }
>> @@ -209,7 +207,8 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde
>> return NULL;
>> }
>> -static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
>> +static int write_sb_page(struct bitmap *bitmap, struct page *page,
>> + unsigned long index, int wait)
>> {
>> struct md_rdev *rdev;
>> struct block_device *bdev;
>> @@ -224,7 +223,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
>> bdev = (rdev->meta_bdev) ? rdev->meta_bdev : rdev->bdev;
>> - if (page->index == store->file_pages-1) {
>> + if (index == store->file_pages-1) {
>> int last_page_size = store->bytes & (PAGE_SIZE-1);
>> if (last_page_size == 0)
>> last_page_size = PAGE_SIZE;
>> @@ -236,8 +235,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
>> */
>> if (mddev->external) {
>> /* Bitmap could be anywhere. */
>> - if (rdev->sb_start + offset + (page->index
>> - * (PAGE_SIZE/512))
>> + if (rdev->sb_start + offset + index * PAGE_SECTORS
>> > rdev->data_offset
>> &&
>> rdev->sb_start + offset
>> @@ -247,7 +245,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
>> } else if (offset < 0) {
>> /* DATA BITMAP METADATA */
>> if (offset
>> - + (long)(page->index * (PAGE_SIZE/512))
>> + + (long)(index * PAGE_SECTORS)
>> + size/512 > 0)
>> /* bitmap runs in to metadata */
>> goto bad_alignment;
>> @@ -259,7 +257,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
>> /* METADATA BITMAP DATA */
>> if (rdev->sb_start
>> + offset
>> - + page->index*(PAGE_SIZE/512) + size/512
>> + + index * PAGE_SECTORS + size/512
>> > rdev->data_offset)
>> /* bitmap runs in to data */
>> goto bad_alignment;
>> @@ -268,7 +266,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
>> }
>> md_super_write(mddev, rdev,
>> rdev->sb_start + offset
>> - + page->index * (PAGE_SIZE/512),
>> + + index * PAGE_SECTORS,
>> size,
>> page);
>> }
>> @@ -285,12 +283,13 @@ static void md_bitmap_file_kick(struct bitmap *bitmap);
>> /*
>> * write out a page to a file
>> */
>> -static void write_page(struct bitmap *bitmap, struct page *page, int wait)
>> +static void write_page(struct bitmap *bitmap, struct page *page,
>> + unsigned long index, int wait)
>> {
>> struct buffer_head *bh;
>> if (bitmap->storage.file == NULL) {
>> - switch (write_sb_page(bitmap, page, wait)) {
>> + switch (write_sb_page(bitmap, page, index, wait)) {
>> case -EINVAL:
>> set_bit(BITMAP_WRITE_ERROR, &bitmap->flags);
>> }
>> @@ -399,7 +398,6 @@ static int read_page(struct file *file, unsigned long index,
>> blk_cur++;
>> bh = bh->b_this_page;
>> }
>> - page->index = index;
>> wait_event(bitmap->write_wait,
>> atomic_read(&bitmap->pending_writes)==0);
>> @@ -472,7 +470,7 @@ void md_bitmap_update_sb(struct bitmap *bitmap)
>> sb->sectors_reserved = cpu_to_le32(bitmap->mddev->
>> bitmap_info.space);
>> kunmap_atomic(sb);
>> - write_page(bitmap, bitmap->storage.sb_page, 1);
>> + write_page(bitmap, bitmap->storage.sb_page, bitmap->storage.sb_index, 1);
>
> I guess it is fine for sb_page now.
>
> [...]
>
>> @@ -1027,7 +1024,7 @@ void md_bitmap_unplug(struct bitmap *bitmap)
>> "md bitmap_unplug");
>> }
>> clear_page_attr(bitmap, i, BITMAP_PAGE_PENDING);
>> - write_page(bitmap, bitmap->storage.filemap[i], 0);
>> + write_page(bitmap, bitmap->storage.filemap[i], i, 0);
>
> But for filemap page, I am not sure the above is correct.
>
>> writing = 1;
>> }
>> }
>> @@ -1137,7 +1134,7 @@ static int md_bitmap_init_from_disk(struct bitmap *bitmap, sector_t start)
>> memset(paddr + offset, 0xff,
>> PAGE_SIZE - offset);
>> kunmap_atomic(paddr);
>> - write_page(bitmap, page, 1);
>> + write_page(bitmap, page, index, 1);
>
> Ditto.
>
>> ret = -EIO;
>> if (test_bit(BITMAP_WRITE_ERROR,
>> @@ -1336,7 +1333,7 @@ void md_bitmap_daemon_work(struct mddev *mddev)
>> if (bitmap->storage.filemap &&
>> test_and_clear_page_attr(bitmap, j,
>> BITMAP_PAGE_NEEDWRITE)) {
>> - write_page(bitmap, bitmap->storage.filemap[j], 0);
>> + write_page(bitmap, bitmap->storage.filemap[j], j, 0);
>
> Ditto.
>
>
>> }
>> }
>> diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
>> index cfd7395de8..6e820eea32 100644
>> --- a/drivers/md/md-bitmap.h
>> +++ b/drivers/md/md-bitmap.h
>> @@ -207,6 +207,7 @@ struct bitmap {
>> * w/ filemap pages */
>> unsigned long file_pages; /* number of pages in the file*/
>> unsigned long bytes; /* total bytes in the bitmap */
>> + unsigned long sb_index; /* sb page index */
>
> I guess it resolve the issue for sb_page, and we might need to do similar things
> for filemap as well if I am not misunderstood.
>
> Thanks,
> Guoqing
>
next prev parent reply other threads:[~2021-10-15 8:59 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-13 16:00 [PATCH 0/5] Minor mm/struct page work Kent Overstreet
2021-10-13 16:00 ` [PATCH 1/5] mm: Make free_area->nr_free per migratetype Kent Overstreet
2021-10-13 16:33 ` David Hildenbrand
2021-10-14 14:45 ` Kent Overstreet
2021-10-18 7:44 ` Vlastimil Babka
2021-10-15 8:24 ` [mm] 1609369623: BUG:kernel_NULL_pointer_dereference,address kernel test robot
2021-10-13 16:00 ` [PATCH 2/5] mm: Introduce struct page_free_list Kent Overstreet
2021-10-13 16:00 ` [PATCH 3/5] mm/page_reporting: Improve control flow Kent Overstreet
2021-10-13 16:00 ` [PATCH 4/5] md: Kill usage of page->index Kent Overstreet
2021-10-14 8:02 ` Guoqing Jiang
2021-10-14 8:58 ` heming.zhao
2021-10-14 14:30 ` [PATCH v2] " Kent Overstreet
2021-10-15 3:01 ` Guoqing Jiang
2021-10-15 8:59 ` heming.zhao [this message]
2021-10-13 16:00 ` [PATCH 5/5] brd: " Kent Overstreet
2021-10-14 14:27 ` Matthew Wilcox
2021-10-14 15:09 ` David Hildenbrand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2c33ebaf-66d2-3138-9e75-ab366b395ad9@suse.com \
--to=heming.zhao@suse.com \
--cc=akpm@linux-foundation.org \
--cc=alexander.h.duyck@linux.intel.com \
--cc=axboe@kernel.dk \
--cc=guoqing.jiang@linux.dev \
--cc=kent.overstreet@gmail.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-raid@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®