Re: [PATCH v3 06/20] drbd: add DAX/PMEM support for metadata access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [High] drbd: partition offset ignored in drbd_dax_open() and
  drbd_dax_map()
- [High] drbd: sub-page offset and page-count truncation when PAGE_SIZE
  > 4096
- [High] drbd: missing cache writeback and ordering in
  drbd_dax_al_initialize()
- [High] drbd: __LC_DIRTY never cleared in drbd_dax_al_begin_io_commit()
- [High] drbd: link failure and wrong Kconfig dependency on
  CONFIG_DEV_DAX_PMEM
- [Medium] drbd: signed 32-bit overflow of al_slots in
  drbd_dax_al_initialize()
- [Medium] drbd: uncommitted and unlocked act_log read in
  drbd_dax_al_initialize()
--

commit 78fd73a53d10bb7aa94cbf5a0d1278098930c4df
Author: Christoph Böhmwalder <christoph.boehmwalder@linbit.com>

drbd: add DAX/PMEM support for metadata access

This commit adds DAX/PMEM support for DRBD metadata access, allowing the
superblock, activity log, and resync bitmap to be mapped and updated
directly in persistent memory.

> diff --git a/drivers/block/drbd/Makefile b/drivers/block/drbd/Makefile
> index 9f05f74e619c1..8ec1ad3604499 100644
> --- a/drivers/block/drbd/Makefile
> +++ b/drivers/block/drbd/Makefile
> @@ -5,6 +5,7 @@ drbd-y += drbd_main.o drbd_strings.o drbd_nl.o
>  drbd-y += drbd_interval.o drbd_state.o
>  drbd-y += drbd_nl_gen.o
>  drbd-y += drbd_transport.o
> +drbd-$(CONFIG_DEV_DAX_PMEM) += drbd_dax_pmem.o
>  drbd-$(CONFIG_DEBUG_FS) += drbd_debugfs.o
>  
>  obj-$(CONFIG_BLK_DEV_DRBD)     += drbd.o

[Severity: High]
Does the build fail to link when CONFIG_BLK_DEV_DRBD=y and
CONFIG_DEV_DAX_PMEM=m?  In that configuration, this line expands to
drbd-m += drbd_dax_pmem.o, which Kbuild ignores when linking built-in
drbd.o, while IS_ENABLED(CONFIG_DEV_DAX_PMEM) in drbd_dax_pmem.h still
evaluates to true and declares drbd_dax_*() as external functions instead
of stubs.

In addition, drbd_dax_open() looks up the DAX device via
fs_dax_get_by_bdev(), which is gated on CONFIG_FS_DAX rather than
CONFIG_DEV_DAX_PMEM (the /dev/daxX.Y character device driver), so
drbd_dax_open() always returns -ENODEV if CONFIG_FS_DAX is disabled.

> diff --git a/drivers/block/drbd/drbd_dax_pmem.c b/drivers/block/drbd/drbd_dax_pmem.c
> new file mode 100644
> index 0000000000000..1fb22e57cf5d0
> --- /dev/null
> +++ b/drivers/block/drbd/drbd_dax_pmem.c
[ ... ]
> +static pgoff_t md_sector_to_pgoff(sector_t sector)
> +{
> +	return sector >> (PAGE_SHIFT - SECTOR_SHIFT);
> +}
> +
> +static long md_sectors_to_pages(sector_t nr_sectors)
> +{
> +	return nr_sectors >> (PAGE_SHIFT - SECTOR_SHIFT);
> +}
> +
> +/* Signed, since the bitmap offset relative to the superblock may be negative. */
> +static long md_sector_offset_to_bytes(long sector_offset)
> +{
> +	return sector_offset * SECTOR_SIZE;
> +}
> +
> +static int map_superblock_for_dax(struct drbd_backing_dev *bdev, struct dax_device *dax_dev)
> +{
> +	long want = 1;
> +	pgoff_t pgoff = md_sector_to_pgoff(bdev->md.md_offset);
> +	void *kaddr;
> +	long len;
> +	int id;
> +
> +	id = dax_read_lock();
> +	len = dax_direct_access(dax_dev, pgoff, want, DAX_ACCESS, &kaddr, NULL);
> +	dax_read_unlock(id);
> +
> +	if (len < want)
> +		return -EIO;
> +
> +	bdev->md_on_pmem = kaddr;
> +
> +	return 0;
> +}
> +
> +/**
> + * drbd_dax_open() - Open device for dax and map metadata superblock
> + * @bdev: backing device to be opened
> + */
> +int drbd_dax_open(struct drbd_backing_dev *bdev)
> +{
> +	struct dax_device *dax_dev;
> +	int err;
> +	u64 part_off;
> +
> +	dax_dev = fs_dax_get_by_bdev(bdev->md_bdev, &part_off, NULL, NULL);
> +	if (!dax_dev)
> +		return -ENODEV;
> +
> +	err = map_superblock_for_dax(bdev, dax_dev);
> +	if (!err)
> +		bdev->dax_dev = dax_dev;
> +	else
> +		put_dax(dax_dev);
> +
> +	return err;
> +}
> +
> +void drbd_dax_close(struct drbd_backing_dev *bdev)
> +{
> +	put_dax(bdev->dax_dev);
> +}
> +
> +/**
> + * drbd_dax_map() - Map metadata for dax
> + * @bdev: backing device whose metadata is to be mapped
> + */
> +int drbd_dax_map(struct drbd_backing_dev *bdev)
> +{
> +	struct dax_device *dax_dev = bdev->dax_dev;
> +	sector_t first_sector = drbd_md_first_sector(bdev);
> +	sector_t al_sector = bdev->md.md_offset + bdev->md.al_offset;
> +	long want = md_sectors_to_pages(drbd_md_last_sector(bdev) + 1 - first_sector);
> +	pgoff_t pgoff = md_sector_to_pgoff(first_sector);
> +	long md_offset_byte = md_sector_offset_to_bytes(bdev->md.md_offset - first_sector);
> +	long al_offset_byte = md_sector_offset_to_bytes(al_sector - first_sector);
> +	void *kaddr;
> +	long len;
> +	int id;
> +
> +	id = dax_read_lock();
> +	len = dax_direct_access(dax_dev, pgoff, want, DAX_ACCESS, &kaddr, NULL);
> +	dax_read_unlock(id);
> +
> +	if (len < want)
> +		return -EIO;
> +
> +	bdev->md_on_pmem = kaddr + md_offset_byte;
> +	bdev->al_on_pmem = kaddr + al_offset_byte;
> +
> +	return 0;
> +}

[Severity: High]
Does discarding part_off in drbd_dax_open() cause DRBD to read and write
metadata at the wrong offset when bdev->md_bdev is a partition?

fs_dax_get_by_bdev() returns the dax_device for the whole disk and writes
the partition's starting byte offset into part_off.  Because
map_superblock_for_dax() and drbd_dax_map() pass partition-relative
pgoff values to dax_direct_access() without adding part_off, metadata
accesses hit the beginning of the parent PMEM disk instead of the
partition, corrupting the partition table or earlier partitions.

[Severity: High]
Can md_sector_to_pgoff() and md_sectors_to_pages() misalign pointers and
truncate the mapping when PAGE_SIZE > 4096 (such as on arm64 or powerpc
with 64 KiB pages)?

DRBD metadata offsets and sizes are 4 KiB-aligned, not PAGE_SIZE-aligned;
for example, internal metadata places the 4 KiB superblock in the last 8
sectors of the device.  In map_superblock_for_dax(), md_sector_to_pgoff()
shifts off the sub-page bits of bdev->md.md_offset, so kaddr points to the
start of the page rather than the superblock.  In drbd_dax_map(), kaddr
also starts at the page boundary pgoff << PAGE_SHIFT, but md_offset_byte
and al_offset_byte are calculated relative to first_sector instead of the
start of pgoff, and md_sectors_to_pages() right-shifts the sector count
down (yielding want == 0 if the metadata region is smaller than a page).

[ ... ]

> +void drbd_dax_al_begin_io_commit(struct drbd_device *device)
> +{
> +	struct lc_element *e;
> +
> +	spin_lock_irq(&device->al_lock);
> +
> +	list_for_each_entry(e, &device->act_log->to_be_changed, list)
> +		drbd_dax_al_update(device, e);
> +
> +	lc_committed(device->act_log);
> +
> +	spin_unlock_irq(&device->al_lock);
> +}

[Severity: High]
Does leaving __LC_DIRTY set in device->act_log->flags after
drbd_dax_al_begin_io_commit() cause later activity log locking to hang?

When an activity log lookup misses, __lc_get() sets __LC_DIRTY in
lc->flags.  lc_committed() clears pending_changes, but __LC_DIRTY is only
cleared by lc_unlock().  Unlike drbd_al_begin_io_commit(), this function
does not acquire drbd_al_try_lock_for_transaction(), call
lc_unlock(device->act_log), or wake device->al_wait.  Once __LC_DIRTY is
set, lc_try_lock(device->act_log) (which requires lc->flags == 0) always
fails, so check_al_extents() hangs forever in wait_event(device->al_wait,
drbd_al_try_lock(device)) and drbd_try_suspend_al() always fails.

> +int drbd_dax_al_initialize(struct drbd_device *device)
> +{
> +	struct al_on_pmem *al_on_pmem = device->ldev->al_on_pmem;
> +	__be32 *slots = al_on_pmem->slots;
> +	/* The activity log area holds the magic followed by the slots. */
> +	int i, al_slots = device->ldev->md.al_size_4k * (SZ_4K / sizeof(*slots)) - 1;
> +
> +	al_on_pmem->magic = cpu_to_be32(DRBD_AL_PMEM_MAGIC);
> +	/*
> +	 * Initialize all slots rather than just the configured number in case
> +	 * the configuration is later changed.
> +	 */
> +	for (i = 0; i < al_slots; i++) {
> +		unsigned int extent_nr = i < device->act_log->nr_elements ?
> +			lc_element_by_index(device->act_log, i)->lc_number :
> +			LC_FREE;
> +		slots[i] = cpu_to_be32(extent_nr);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does drbd_dax_al_initialize() need to flush the initialized activity log
with arch_wb_cache_pmem() and write al_on_pmem->magic only after slots[]
are persisted?

Unlike drbd_dax_al_update(), drbd_dax_al_initialize() never calls
arch_wb_cache_pmem(), and it stores DRBD_AL_PMEM_MAGIC before populating
slots[].  If a crash or power failure occurs before the slot cache lines
are evicted from volatile CPU caches, the activity log on PMEM can have a
valid magic header with uninitialized or stale slots.

[Severity: Medium]
Can al_slots overflow a signed 32-bit int when al_size_4k is larger than
8 GiB?

check_activity_log_stripe_size() allows al_size_4k up to 4,194,304
(16 GiB).  Multiplying by SZ_4K / sizeof(*slots) (1024) produces up to
0xffffffff, which becomes negative when stored in int al_slots and causes
the loop to be skipped without initializing any slots.

[Severity: Medium]
Can drbd_dax_al_initialize() write stale extent numbers to PMEM when
device->act_log->to_be_changed has uncommitted elements?

Unlike the non-DAX drbd_al_initialize() path, which holds device->al_lock,
writes lc_new_number for entries on to_be_changed, and calls
lc_committed(device->act_log), drbd_dax_al_initialize() reads lc_number
without holding device->al_lock or committing to_be_changed.  Any pending
element on to_be_changed has its old lc_number written to PMEM and remains
on to_be_changed after the caller unlocks device->act_log.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110162646.3028472-1-christoph.boehmwalder@linbit.com?part=6