From: Dan Williams Subject: Re: [PATCH 4/5] dax: use sb_issue_zerout instead of calling dax_clear_sectors Date: Mon, 28 Mar 2016 16:34:16 -0700 Message-ID: References: <1458861450-17705-1-git-send-email-vishal.l.verma@intel.com> <1458861450-17705-5-git-send-email-vishal.l.verma@intel.com> <1458939796.5501.8.camel@intel.com> <1459195288.15523.3.camel@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: "linux-block@vger.kernel.org" , "xfs@oss.sgi.com" , "linux-mm@kvack.org" , "viro@zeniv.linux.org.uk" , "akpm@linux-foundation.org" , "axboe@fb.com" , "linux-nvdimm@lists.01.org" , "linux-fsdevel@vger.kernel.org" , "ross.zwisler@linux.intel.com" , "linux-ext4@vger.kernel.org" , "Wilcox, Matthew R" , "david@fromorbit.com" , "jack@suse.cz" To: "Verma, Vishal L" Return-path: In-Reply-To: <1459195288.15523.3.camel@intel.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-ext4.vger.kernel.org On Mon, Mar 28, 2016 at 1:01 PM, Verma, Vishal L wrote: > On Fri, 2016-03-25 at 14:20 -0700, Dan Williams wrote: >> On Fri, Mar 25, 2016 at 2:03 PM, Verma, Vishal L >> wrote: >> > >> > On Fri, 2016-03-25 at 11:47 -0700, Dan Williams wrote: >> > > >> > > On Thu, Mar 24, 2016 at 4:17 PM, Vishal Verma > > > el.c >> > > om> wrote: >> > > > >> > > > >> > > > From: Matthew Wilcox >> > > > >> > > > dax_clear_sectors() cannot handle poisoned blocks. These must >> > > > be >> > > > zeroed using the BIO interface instead. Convert ext2 and XFS >> > > > to >> > > > use >> > > > only sb_issue_zerout(). >> > > > >> > > > Signed-off-by: Matthew Wilcox >> > > > [vishal: Also remove the dax_clear_sectors function entirely] >> > > > Signed-off-by: Vishal Verma >> > > > --- >> > > > fs/dax.c | 32 -------------------------------- >> > > > fs/ext2/inode.c | 7 +++---- >> > > > fs/xfs/xfs_bmap_util.c | 9 --------- >> > > > include/linux/dax.h | 1 - >> > > > 4 files changed, 3 insertions(+), 46 deletions(-) >> > > > >> > > > diff --git a/fs/dax.c b/fs/dax.c >> > > > index bb7e9f8..a30481e 100644 >> > > > --- a/fs/dax.c >> > > > +++ b/fs/dax.c >> > > > @@ -78,38 +78,6 @@ struct page *read_dax_sector(struct >> > > > block_device >> > > > *bdev, sector_t n) >> > > > return page; >> > > > } >> > > > >> > > > -/* >> > > > - * dax_clear_sectors() is called from within transaction >> > > > context >> > > > from XFS, >> > > > - * and hence this means the stack from this point must follow >> > > > GFP_NOFS >> > > > - * semantics for all operations. >> > > > - */ >> > > > -int dax_clear_sectors(struct block_device *bdev, sector_t >> > > > _sector, >> > > > long _size) >> > > > -{ >> > > > - struct blk_dax_ctl dax = { >> > > > - .sector = _sector, >> > > > - .size = _size, >> > > > - }; >> > > > - >> > > > - might_sleep(); >> > > > - do { >> > > > - long count, sz; >> > > > - >> > > > - count = dax_map_atomic(bdev, &dax); >> > > > - if (count < 0) >> > > > - return count; >> > > > - sz = min_t(long, count, SZ_128K); >> > > > - clear_pmem(dax.addr, sz); >> > > > - dax.size -= sz; >> > > > - dax.sector += sz / 512; >> > > > - dax_unmap_atomic(bdev, &dax); >> > > > - cond_resched(); >> > > > - } while (dax.size); >> > > > - >> > > > - wmb_pmem(); >> > > > - return 0; >> > > > -} >> > > > -EXPORT_SYMBOL_GPL(dax_clear_sectors); >> > > What about the other unwritten extent conversions in the dax >> > > path? >> > > Shouldn't those be converted to block-layer zero-outs as well? >> > Could you point me to where these might be? I thought once we've >> > converted all the zeroout type callers (by removing >> > dax_clear_sectors), >> > and fixed up dax_do_io to try a driver fallback, we've handled all >> > the >> > media error cases in dax.. >> grep for usages of clear_pmem()... which I was hoping to eliminate >> after this change to push zeroing down to the driver. > > Ok, so I looked at these, and it looks like the majority of callers of > clear_pmem are from the fault path (either pmd or regular), and in > those cases we should be 'protected', as we would have failed at a > prior step (dax_map_atomic). Seems kind of sad to fail the fault due to a bad block when we were going to zero it anyway, right? I'm not seeing a compelling reason to keep any zeroing in fs/dax.c.