Re: [PATCH v5 11/11] md/md-llbitmap: introduce new lockless bitmap

[Yu Kuai]

Hi, Xiao

在 2025/08/07 11:57, Xiao Ni 写道:
> > +/* set all the bits in the subpage as dirty */
> > +static void llbitmap_infect_dirty_bits(struct llbitmap *llbitmap,
> > +                                      struct llbitmap_page_ctl *pctl,
> > +                                      unsigned int block, unsigned int offset)
> > +{
> > +       bool level_456 = raid_is_456(llbitmap->mddev);
> > +       unsigned int io_size = llbitmap->io_size;
> > +       int pos;
> > +
> > +       for (pos = block * io_size; pos < (block + 1) * io_size; pos++) {
> > +               if (pos == offset)
> > +                       continue;
> It looks like it doesn't need to pass the argument offset to this
> function. The pctl->state[offset] must be BitDirty or BitNeedSync. So
> the following switch/case can skip it. So it can save hundreds
> comparing pos with offset here.
Ok.
> > +
> > +               switch (pctl->state[pos]) {
> > +               case BitUnwritten:
> > +                       pctl->state[pos] = level_456 ? BitNeedSync : BitDirty;
> > +                       break;
> > +               case BitClean:
> > +                       pctl->state[pos] = BitDirty;
> > +                       break;
> > +               };
> > +       }
> > +
> > +}
> > +static void llbitmap_write(struct llbitmap *llbitmap, enum llbitmap_state state,
> > +                          loff_t pos)
> > +{
> > +       unsigned int idx;
> > +       unsigned int offset;
> How about change offset to bit?

Yes, and the follong name about bit.
> > +static void llbitmap_write_page(struct llbitmap *llbitmap, int idx)
> > +{
> > +       struct page *page = llbitmap->pctl[idx]->page;
> > +       struct mddev *mddev = llbitmap->mddev;
> > +       struct md_rdev *rdev;
> > +       int bit;
> It's better to change name "bit" to "block"
> > +static int llbitmap_init(struct llbitmap *llbitmap)
> > +{
> > +       struct mddev *mddev = llbitmap->mddev;
> > +       sector_t blocks = mddev->resync_max_sectors;
> > +       unsigned long chunksize = MIN_CHUNK_SIZE;
> > +       unsigned long chunks = DIV_ROUND_UP(blocks, chunksize);
> > +       unsigned long space = mddev->bitmap_info.space << SECTOR_SHIFT;
> > +       int ret;
> > +
> > +       while (chunks > space) {
> > +               chunksize = chunksize << 1;
> > +               chunks = DIV_ROUND_UP(blocks, chunksize);
> > +       }
> > +
> > +       llbitmap->barrier_idle = DEFAULT_BARRIER_IDLE;
> > +       llbitmap->chunkshift = ffz(~chunksize);
> > +       llbitmap->chunksize = chunksize;
> > +       llbitmap->chunks = chunks;
> > +       mddev->bitmap_info.daemon_sleep = DEFAULT_DAEMON_SLEEP;
> > +
> > +       ret = llbitmap_cache_pages(llbitmap);
> > +       if (ret)
> > +               return ret;
> > +
> > +       llbitmap_state_machine(llbitmap, 0, llbitmap->chunks - 1, BitmapActionInit);
> > +       return 0;
> > +}
> There is a problem, if array is created with --assume-clean, it
> doesn't need to start sync. And it doesn't have the chance to sync
> llbitmap superblock to member disks. So it can't get the right bitmap
> superblock information which is calculated here after a power off. It
> needs to sync the superblock here.
Yes, you're right. The mdraid bitmap metadata is written by mdadm,
dm-raid  bitmap metadata is write in kernel by md_update_sb(), llbitmap
is missing a call to llbitmap_write_sb() after initializing, if
md_update_sb() is never triggered, the bitmap metadata will be lost.

> > +
> > +static int llbitmap_create(struct mddev *mddev)
> > +{
> > +       struct llbitmap *llbitmap;
> > +       int ret;
> > +
> > +       ret = llbitmap_check_support(mddev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       llbitmap = kzalloc(sizeof(*llbitmap), GFP_KERNEL);
> > +       if (!llbitmap)
> > +               return -ENOMEM;
> > +
> > +       llbitmap->mddev = mddev;
> > +       llbitmap->io_size = bdev_logical_block_size(mddev->gendisk->part0);
> > +       llbitmap->blocks_per_page = PAGE_SIZE / llbitmap->io_size;
> > +
> > +       timer_setup(&llbitmap->pending_timer, llbitmap_pending_timer_fn, 0);
> > +       INIT_WORK(&llbitmap->daemon_work, md_llbitmap_daemon_fn);
> > +       atomic_set(&llbitmap->behind_writes, 0);
> > +       init_waitqueue_head(&llbitmap->behind_wait);
> > +
> > +       mutex_lock(&mddev->bitmap_info.mutex);
> > +       mddev->bitmap = llbitmap;
> > +       ret = llbitmap_read_sb(llbitmap);
> > +       mutex_unlock(&mddev->bitmap_info.mutex);
> > +       if (ret)
> > +               goto err_out;
> > +
> > +       return 0;
> > +
> > +err_out:
> > +       kfree(llbitmap);
> mddev->bitmap = NULL. If not,
> md_run->md_bitmap_destroy->llbitmap_destroy will free it again.
Yes.

> > +static void llbitmap_start_write(struct mddev *mddev, sector_t offset,
> > +                                unsigned long sectors)
> > +{
> > +       struct llbitmap *llbitmap = mddev->bitmap;
> > +       unsigned long start = offset >> llbitmap->chunkshift;
> > +       unsigned long end = (offset + sectors - 1) >> llbitmap->chunkshift;
> > +       int page_start = (start + BITMAP_DATA_OFFSET) >> PAGE_SHIFT;
> > +       int page_end = (end + BITMAP_DATA_OFFSET) >> PAGE_SHIFT;
> > +
> > +       llbitmap_state_machine(llbitmap, start, end, BitmapActionStartwrite);
> > +
> > +
> Two lines here, just need one.
> > +/*
> > + * Force to write all bitmap pages to disk, called when stopping the array, or
> > + * every daemon_sleep seconds when sync_thread is running.
> > + */
> > +static void __llbitmap_flush(struct mddev *mddev)
> > +{
> > +       struct llbitmap *llbitmap = mddev->bitmap;
> > +       struct blk_plug plug;
> > +       int i;
> > +
> > +       blk_start_plug(&plug);
> > +       for (i = 0; i < llbitmap->nr_pages; i++) {
> > +               struct llbitmap_page_ctl *pctl = llbitmap->pctl[i];
> > +
> > +               /* mark all bits as dirty */
> "mark all blocks as dirty" is better?
>
> > +static void llbitmap_write_sb(struct llbitmap *llbitmap)
> > +{
> > +       int nr_bits = DIV_ROUND_UP(BITMAP_DATA_OFFSET, llbitmap->io_size);
> s/nr_bits/nr_blocks/g

Thanks again for the review!
Kuai