Re: [PATCH v2 2/5] dax: increase granularity of dax_clear_blocks() operations

["Williams, Dan J" <dan.j.williams@intel.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2680 bytes --]

On Thu, 2015-10-22 at 17:04 -0400, Jeff Moyer wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > dax_clear_blocks is currently performing a cond_resched() after every
> > PAGE_SIZE memset.  We need not check so frequently, for example md-raid
> > only calls cond_resched() at stripe granularity.  Also, in preparation
> > for introducing a dax_map_atomic() operation that temporarily pins a dax
> > mapping move the call to cond_resched() to the outer loop.
> 
> There's nothing wrong with the mechanics here, but why bother?  I only
> see 1 caller in the kernel, and that caller passes in
> 1<<inode->i_blkbits for the size (so 1 page or less).  Did you plan to
> add other callers?  I don't see them in this particular patch set.
> 
> Again, I'm not taking issue with the patch, I'm just wondering what
> motivated the change.

The motivation is the subsequent patch to wrap all touches of pmem
within a dax_map_atomic() / dax_unmap_atomic() pairing.  If I just do
the straightforward conversion of this function to dax_map_atomic() it
looks something like this:


> diff --git a/fs/dax.c b/fs/dax.c
> index 5dc33d788d50..fa2a2a255d3a 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -40,9 +40,9 @@ int dax_clear_blocks(struct inode *inode, sector_t block, long size)
>               unsigned long pfn;
>               long count;
>  
> -             count = bdev_direct_access(bdev, sector, &addr, &pfn, size);
> -             if (count < 0)
> -                     return count;
> +             addr = __dax_map_atomic(bdev, sector, size, &pfn, &count);
> +             if (IS_ERR(addr))
> +                     return PTR_ERR(addr);
>               BUG_ON(size < count);
>               while (count > 0) {
>                       unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
> @@ -56,6 +56,7 @@ int dax_clear_blocks(struct inode *inode, sector_t block, long size)
>                       sector += pgsz / 512;
>                       cond_resched();
>               }
> +             dax_unmap_atomic(bdev, addr);
>       } while (size);
>  
>       wmb_pmem();

The problem is that intervening call to cond_resched().  I later want to
inject an rcu_read_lock()/unlock() pair to allow flushing active
dax_map_atomic() usages at driver teardown time [1].  But, I think the
patch stands alone as a cleanup outside of that admittedly hidden
motivation.

[1]: "mm, pmem: devm_memunmap_pages(), truncate and unmap ZONE_DEVICE
pages"
https://lists.01.org/pipermail/linux-nvdimm/2015-October/002406.html


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥