linux-kernel - Re: [PATCH v2 2/5] dax: increase granularity of dax_clear

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1445554654.17208.14.camel@intel.com>
Date:	Thu, 22 Oct 2015 22:57:37 +0000
From:	"Williams, Dan J" <dan.j.williams@...el.com>
To:	"jmoyer@...hat.com" <jmoyer@...hat.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-nvdimm@...1.01.org" <linux-nvdimm@...1.01.org>,
	"hch@....de" <hch@....de>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"axboe@...com" <axboe@...com>, "jack@...e.com" <jack@...e.com>,
	"david@...morbit.com" <david@...morbit.com>,
	"jack@...e.cz" <jack@...e.cz>
Subject: Re: [PATCH v2 2/5] dax: increase granularity of dax_clear_blocks()
 operations

On Thu, 2015-10-22 at 17:04 -0400, Jeff Moyer wrote:
> Dan Williams <dan.j.williams@...el.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