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]
Date:   Fri, 15 Oct 2021 16:59:11 +0800
From:   "heming.zhao@...e.com" <heming.zhao@...e.com>
To:     Guoqing Jiang <guoqing.jiang@...ux.dev>,
        Kent Overstreet <kent.overstreet@...il.com>
CC:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        akpm@...ux-foundation.org, linux-raid@...r.kernel.org,
        linux-block@...r.kernel.org, axboe@...nel.dk,
        alexander.h.duyck@...ux.intel.com
Subject: Re: [PATCH v2] md: Kill usage of page->index

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@...e.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@...il.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
> 

Powered by blists - more mailing lists