| 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
| ||
|
Message-ID: <84baf54c-338b-1822-c631-cf7d0da1f61a@huaweicloud.com>
Date: Tue, 13 Aug 2024 15:33:55 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Mariusz Tkaczyk <mariusz.tkaczyk@...ux.intel.com>,
Yu Kuai <yukuai1@...weicloud.com>
Cc: song@...nel.org, linux-kernel@...r.kernel.org,
linux-raid@...r.kernel.org, yi.zhang@...wei.com, yangerkun@...wei.com,
"yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH RFC -next 07/26] md/md-bitmap: merge md_bitmap_update_sb()
into bitmap_operations
Hi,
在 2024/08/13 15:17, Mariusz Tkaczyk 写道:
> On Sat, 10 Aug 2024 10:08:35 +0800
> Yu Kuai <yukuai1@...weicloud.com> wrote:
>
>> From: Yu Kuai <yukuai3@...wei.com>
>>
>> So that the implementation won't be exposed, and it'll be possible
>> to invent a new bitmap by replacing bitmap_operations.
>
> Please update commit message.
>
>>
>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
>> ---
>> drivers/md/md-bitmap.c | 15 ++++++++-------
>> drivers/md/md-bitmap.h | 11 ++++++++++-
>> drivers/md/md-cluster.c | 3 ++-
>> drivers/md/md.c | 4 ++--
>> 4 files changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>> index 0ff733756043..b34f13aa2697 100644
>> --- a/drivers/md/md-bitmap.c
>> +++ b/drivers/md/md-bitmap.c
>> @@ -472,7 +472,7 @@ static void md_bitmap_wait_writes(struct bitmap *bitmap)
>>
>>
>> /* update the event counter and sync the superblock to disk */
>> -void md_bitmap_update_sb(struct bitmap *bitmap)
>> +static void bitmap_update_sb(struct bitmap *bitmap)
>> {
>> bitmap_super_t *sb;
>>
>> @@ -510,7 +510,6 @@ void md_bitmap_update_sb(struct bitmap *bitmap)
>> write_sb_page(bitmap, bitmap->storage.sb_index,
>> bitmap->storage.sb_page, 1);
>> }
>> -EXPORT_SYMBOL(md_bitmap_update_sb);
>>
>> /* print out the bitmap file superblock */
>> static void bitmap_print_sb(struct bitmap *bitmap)
>> @@ -893,7 +892,7 @@ static void md_bitmap_file_unmap(struct bitmap_storage
>> *store) static void md_bitmap_file_kick(struct bitmap *bitmap)
>> {
>> if (!test_and_set_bit(BITMAP_STALE, &bitmap->flags)) {
>> - md_bitmap_update_sb(bitmap);
>> + bitmap_update_sb(bitmap);
>>
>> if (bitmap->storage.file) {
>> pr_warn("%s: kicking failed bitmap file %pD4 from
>> array!\n", @@ -1796,7 +1795,7 @@ static void bitmap_flush(struct mddev *mddev)
>> md_bitmap_daemon_work(mddev);
>> if (mddev->bitmap_info.external)
>> md_super_wait(mddev);
>> - md_bitmap_update_sb(bitmap);
>> + bitmap_update_sb(bitmap);
>> }
>>
>> /*
>> @@ -2014,7 +2013,7 @@ static int bitmap_load(struct mddev *mddev)
>> mddev_set_timeout(mddev, mddev->bitmap_info.daemon_sleep, true);
>> md_wakeup_thread(mddev->thread);
>>
>> - md_bitmap_update_sb(bitmap);
>> + bitmap_update_sb(bitmap);
>
> You changed function name here and it is harmful for git blame. What is the
> reason behind that? it must be described in commit message to help Song making
> the decision if it is worthy merging or not.
>
>>
>> if (test_bit(BITMAP_WRITE_ERROR, &bitmap->flags))
>> err = -EIO;
>> @@ -2075,7 +2074,7 @@ int md_bitmap_copy_from_slot(struct mddev *mddev, int
>> slot, }
>>
>> if (clear_bits) {
>> - md_bitmap_update_sb(bitmap);
>> + bitmap_update_sb(bitmap);
>> /* BITMAP_PAGE_PENDING is set, but bitmap_unplug needs
>> * BITMAP_PAGE_DIRTY or _NEEDWRITE to write ... */
>> for (i = 0; i < bitmap->storage.file_pages; i++)
>> @@ -2568,7 +2567,7 @@ backlog_store(struct mddev *mddev, const char *buf,
>> size_t len) mddev_create_serial_pool(mddev, rdev);
>> }
>> if (old_mwb != backlog)
>> - md_bitmap_update_sb(mddev->bitmap);
>> + bitmap_update_sb(mddev->bitmap);
>>
>> mddev_unlock_and_resume(mddev);
>> return len;
>> @@ -2712,6 +2711,8 @@ static struct bitmap_operations bitmap_ops = {
>> .load = bitmap_load,
>> .destroy = bitmap_destroy,
>> .flush = bitmap_flush,
>> +
>> + .update_sb = bitmap_update_sb,
>> };
>>
>> void mddev_set_bitmap_ops(struct mddev *mddev)
>> diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
>> index 935c5dc45b89..29c217630ae5 100644
>> --- a/drivers/md/md-bitmap.h
>> +++ b/drivers/md/md-bitmap.h
>> @@ -239,6 +239,8 @@ struct bitmap_operations {
>> int (*load)(struct mddev *mddev);
>> void (*destroy)(struct mddev *mddev);
>> void (*flush)(struct mddev *mddev);
>> +
>> + void (*update_sb)(struct bitmap *bitmap);
>> };
>>
>> /* the bitmap API */
>> @@ -277,7 +279,14 @@ static inline void md_bitmap_flush(struct mddev *mddev)
>> mddev->bitmap_ops->flush(mddev);
>> }
>>
>> -void md_bitmap_update_sb(struct bitmap *bitmap);
>> +static inline void md_bitmap_update_sb(struct mddev *mddev)
>> +{
>> + if (!mddev->bitmap || !mddev->bitmap_ops->update_sb)
>> + return;
>
> I would like to avoid dead code here. !mddev->bitmap is probably not an option
> an this point in code. !mddev->bitmap_ops->update_sb i not an option because we
> have only one bitmap op. Do I miss something?
mddev->bitmap will be an option for the array with bitmap=none. However,
!mddev->bitmap_ops->update_sb is indeed not an option.
>
> I will stop here for today now to give you a chance to reply, to be sure that
> we are on same page. I see that my comments are similar so it may not be worthy
> to go one by one and repeat same comment. I may miss something important.
Yes, but for the similiar commit message, I'm not sure how to improve
this for now. We don't want a large patch to merge all the ops at once.
Thanks,
Kuai
>
> Thanks
> Mariusz
>
>
> .
>
Powered by blists - more mailing lists