LinuxLists.cc - futher decouple DAX from block devices

2021-10-18 04:41:37

by Dan Williams

[permalink] [raw]

Subject: Re: [PATCH 10/11] dm-stripe: add a stripe_dax_pgoff helper

On Sun, Oct 17, 2021 at 9:41 PM Christoph Hellwig <[email protected]> wrote:
>
> Add a helper to perform the entire remapping for DAX accesses. This
> helper open codes bdev_dax_pgoff given that the alignment checks have
> already been done by the submitting file system and don't need to be
> repeated.

Again, looks good. Kind of embarrassing when the open-coded version is
less LOC than using the helper.

Mike, ack?

>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> drivers/md/dm-stripe.c | 63 ++++++++++--------------------------------
> 1 file changed, 15 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
> index f084607220293..50dba3f39274c 100644
> --- a/drivers/md/dm-stripe.c
> +++ b/drivers/md/dm-stripe.c
> @@ -301,83 +301,50 @@ static int stripe_map(struct dm_target *ti, struct bio *bio)
> }
>
> #if IS_ENABLED(CONFIG_FS_DAX)
> -static long stripe_dax_direct_access(struct dm_target *ti, pgoff_t pgoff,
> - long nr_pages, void **kaddr, pfn_t *pfn)
> +static struct dax_device *stripe_dax_pgoff(struct dm_target *ti, pgoff_t *pgoff)
> {
> - sector_t dev_sector, sector = pgoff * PAGE_SECTORS;
> struct stripe_c *sc = ti->private;
> - struct dax_device *dax_dev;
> struct block_device *bdev;
> + sector_t dev_sector;
> uint32_t stripe;
> - long ret;
>
> - stripe_map_sector(sc, sector, &stripe, &dev_sector);
> + stripe_map_sector(sc, *pgoff * PAGE_SECTORS, &stripe, &dev_sector);
> dev_sector += sc->stripe[stripe].physical_start;
> - dax_dev = sc->stripe[stripe].dev->dax_dev;
> bdev = sc->stripe[stripe].dev->bdev;
>
> - ret = bdev_dax_pgoff(bdev, dev_sector, nr_pages * PAGE_SIZE, &pgoff);
> - if (ret)
> - return ret;
> + *pgoff = (get_start_sect(bdev) + dev_sector) >> PAGE_SECTORS_SHIFT;
> + return sc->stripe[stripe].dev->dax_dev;
> +}
> +
> +static long stripe_dax_direct_access(struct dm_target *ti, pgoff_t pgoff,
> + long nr_pages, void **kaddr, pfn_t *pfn)
> +{
> + struct dax_device *dax_dev = stripe_dax_pgoff(ti, &pgoff);
> +
> return dax_direct_access(dax_dev, pgoff, nr_pages, kaddr, pfn);
> }
>
> static size_t stripe_dax_copy_from_iter(struct dm_target *ti, pgoff_t pgoff,
> void *addr, size_t bytes, struct iov_iter *i)
> {
> - sector_t dev_sector, sector = pgoff * PAGE_SECTORS;
> - struct stripe_c *sc = ti->private;
> - struct dax_device *dax_dev;
> - struct block_device *bdev;
> - uint32_t stripe;
> -
> - stripe_map_sector(sc, sector, &stripe, &dev_sector);
> - dev_sector += sc->stripe[stripe].physical_start;
> - dax_dev = sc->stripe[stripe].dev->dax_dev;
> - bdev = sc->stripe[stripe].dev->bdev;
> + struct dax_device *dax_dev = stripe_dax_pgoff(ti, &pgoff);
>
> - if (bdev_dax_pgoff(bdev, dev_sector, ALIGN(bytes, PAGE_SIZE), &pgoff))
> - return 0;
> return dax_copy_from_iter(dax_dev, pgoff, addr, bytes, i);
> }
>
> static size_t stripe_dax_copy_to_iter(struct dm_target *ti, pgoff_t pgoff,
> void *addr, size_t bytes, struct iov_iter *i)
> {
> - sector_t dev_sector, sector = pgoff * PAGE_SECTORS;
> - struct stripe_c *sc = ti->private;
> - struct dax_device *dax_dev;
> - struct block_device *bdev;
> - uint32_t stripe;
> -
> - stripe_map_sector(sc, sector, &stripe, &dev_sector);
> - dev_sector += sc->stripe[stripe].physical_start;
> - dax_dev = sc->stripe[stripe].dev->dax_dev;
> - bdev = sc->stripe[stripe].dev->bdev;
> + struct dax_device *dax_dev = stripe_dax_pgoff(ti, &pgoff);
>
> - if (bdev_dax_pgoff(bdev, dev_sector, ALIGN(bytes, PAGE_SIZE), &pgoff))
> - return 0;
> return dax_copy_to_iter(dax_dev, pgoff, addr, bytes, i);
> }
>
> static int stripe_dax_zero_page_range(struct dm_target *ti, pgoff_t pgoff,
> size_t nr_pages)
> {
> - int ret;
> - sector_t dev_sector, sector = pgoff * PAGE_SECTORS;
> - struct stripe_c *sc = ti->private;
> - struct dax_device *dax_dev;
> - struct block_device *bdev;
> - uint32_t stripe;
> + struct dax_device *dax_dev = stripe_dax_pgoff(ti, &pgoff);
>
> - stripe_map_sector(sc, sector, &stripe, &dev_sector);
> - dev_sector += sc->stripe[stripe].physical_start;
> - dax_dev = sc->stripe[stripe].dev->dax_dev;
> - bdev = sc->stripe[stripe].dev->bdev;
> -
> - ret = bdev_dax_pgoff(bdev, dev_sector, nr_pages << PAGE_SHIFT, &pgoff);
> - if (ret)
> - return ret;
> return dax_zero_page_range(dax_dev, pgoff, nr_pages);
> }
>
> --
> 2.30.2
>

2021-10-28 01:44:44

by Dan Williams

[permalink] [raw]

Subject: Re: [PATCH 11/11] dax: move bdev_dax_pgoff to fs/dax.c

On Sun, Oct 17, 2021 at 9:41 PM Christoph Hellwig <[email protected]> wrote:
>
> No functional changet, but this will allow for a tighter integration

s/changet/changes/

> with the iomap code, including possible passing the partition offset

s/possible/possibly/

> in the iomap in the future. For now it mostly avoids growing more

s/now/now,/

...all of the above fixed up locally.

Other than that, it looks good to me.

> callers outside of fs/dax.c.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> drivers/dax/super.c | 14 --------------
> fs/dax.c | 13 +++++++++++++
> include/linux/dax.h | 1 -
> 3 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 803942586d1b6..c0910687fbcb2 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -67,20 +67,6 @@ void dax_remove_host(struct gendisk *disk)
> }
> EXPORT_SYMBOL_GPL(dax_remove_host);
>
> -int bdev_dax_pgoff(struct block_device *bdev, sector_t sector, size_t size,
> - pgoff_t *pgoff)
> -{
> - sector_t start_sect = bdev ? get_start_sect(bdev) : 0;
> - phys_addr_t phys_off = (start_sect + sector) * 512;
> -
> - if (pgoff)
> - *pgoff = PHYS_PFN(phys_off);
> - if (phys_off % PAGE_SIZE || size % PAGE_SIZE)
> - return -EINVAL;
> - return 0;
> -}
> -EXPORT_SYMBOL(bdev_dax_pgoff);
> -
> /**
> * dax_get_by_host() - temporary lookup mechanism for filesystem-dax
> * @bdev: block device to find a dax_device for
> diff --git a/fs/dax.c b/fs/dax.c
> index 4e3e5a283a916..eb715363fd667 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -709,6 +709,19 @@ int dax_invalidate_mapping_entry_sync(struct address_space *mapping,
> return __dax_invalidate_entry(mapping, index, false);
> }
>
> +static int bdev_dax_pgoff(struct block_device *bdev, sector_t sector, size_t size,
> + pgoff_t *pgoff)
> +{
> + sector_t start_sect = bdev ? get_start_sect(bdev) : 0;
> + phys_addr_t phys_off = (start_sect + sector) * 512;
> +
> + if (pgoff)
> + *pgoff = PHYS_PFN(phys_off);
> + if (phys_off % PAGE_SIZE || size % PAGE_SIZE)
> + return -EINVAL;
> + return 0;
> +}
> +
> static int copy_cow_page_dax(struct block_device *bdev, struct dax_device *dax_dev,
> sector_t sector, struct page *to, unsigned long vaddr)
> {
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index 439c3c70e347b..324363b798ecd 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -107,7 +107,6 @@ static inline bool daxdev_mapping_supported(struct vm_area_struct *vma,
> #endif
>
> struct writeback_control;
> -int bdev_dax_pgoff(struct block_device *, sector_t, size_t, pgoff_t *pgoff);
> #if IS_ENABLED(CONFIG_FS_DAX)
> int dax_add_host(struct dax_device *dax_dev, struct gendisk *disk);
> void dax_remove_host(struct gendisk *disk);
> --
> 2.30.2
>

2021-10-28 23:53:25

On Thu, Nov 04, 2021 at 11:09:19PM -0400, Theodore Ts'o wrote:
> On Thu, Nov 04, 2021 at 12:04:43PM -0700, Darrick J. Wong wrote:
> > > Note that I've avoided implementing read/write fops for dax devices
> > > partly out of concern for not wanting to figure out shared-mmap vs
> > > write coherence issues, but also because of a bet with Dave Hansen
> > > that device-dax not grow features like what happened to hugetlbfs. So
> > > it would seem mkfs would need to switch to mmap I/O, or bite the
> > > bullet and implement read/write fops in the driver.
> >
> > That ... would require a fair amount of userspace changes, though at
> > least e2fsprogs has pluggable io drivers, which would make mmapping a
> > character device not too awful.
> >
> > xfsprogs would be another story -- porting the buffer cache mignt not be
> > too bad, but mkfs and repair seem to issue pread/pwrite calls directly.
> > Note that xfsprogs explicitly screens out chardevs.
>
> It's not just e2fsprogs and xfsprogs. There's also udev, blkid,
> potententially systemd unit generators to kick off fsck runs, etc.
> There are probably any number of user scripts which assume that file
> systems are mounted on block devices --- for example, by looking at
> the output of lsblk, etc.
>
> Also note that block devices have O_EXCL support to provide locking
> against attempts to run mkfs on a mounted file system. If you move
> dax file systems to be mounted on a character mode device, that would
> have to be replicated as well, etc. So I suspect that a large number
> of subtle things would break, and I'd strongly recommend against going
> down that path.

Agreed. There were reasons we decided to present pmem as "block
device with extra functionality" rather than try to cram all the block
layer functionality (eg submitting BIOs for filesystem metadata) into a
character device. Some of those assumptions might be worth re-examining,
but I haven't seen anything that makes me say "this is obviously better
than what we did at the time".