| Message ID | 20200129210337.GA13630@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] dax: Do not try to clear poison for partial pages | expand |
On Wed, Jan 29, 2020 at 04:03:37PM -0500, Vivek Goyal wrote: > I am looking into getting rid of dependency on block device in dax > path. One such place is __dax_zero_page_range() which checks if > range being zeroed is aligned to block device logical size, then > it calls bdev_issue_zeroout() instead of doing memset(). Calling > blkdev_issue_zeroout() also clears bad blocks and poison if any > in that range. > > This path is used by iomap_zero_range() which in-turn is being > used by filesystems to zero partial filesystem system blocks. > For zeroing full filesystem blocks we seem to be calling > blkdev_issue_zeroout() which clears bad blocks and poison in that > range. > > So this code currently only seems to help with partial filesystem > block zeroing. That too only if truncation/hole_punch happens on > device logical block size boundary. > > To avoid using blkdev_issue_zeroout() in this path, I proposed another > patch here which adds another dax operation to zero out a rangex and > clear poison. I'll have to defer to Dan and other on the poison clearing issue, as it keeps confusing me everytime I look into how it is supposed to work.. But in the end we'll need a path that doesn't realy on the block device to clear poison anyway, so I think a method will ultimatively be needed. That being said this patch looks nice :)
Hello, I haven't seen response to this proposal, unsure if there is a different but related discussion ongoing... I'd like to express my wish: please make it easier for the pmem applications when possible. If kernel does not clear poison when it could legitimately do so, applications have to go through lengths to clear poisons. For Cloud pmem applications that have upper bound on error recovery time, not clearing poison while zeroing-out is quite undesirable. Thanks! -jane On 1/29/20 1:03 PM, Vivek Goyal wrote: > I am looking into getting rid of dependency on block device in dax > path. One such place is __dax_zero_page_range() which checks if > range being zeroed is aligned to block device logical size, then > it calls bdev_issue_zeroout() instead of doing memset(). Calling > blkdev_issue_zeroout() also clears bad blocks and poison if any > in that range. > > This path is used by iomap_zero_range() which in-turn is being > used by filesystems to zero partial filesystem system blocks. > For zeroing full filesystem blocks we seem to be calling > blkdev_issue_zeroout() which clears bad blocks and poison in that > range. > > So this code currently only seems to help with partial filesystem > block zeroing. That too only if truncation/hole_punch happens on > device logical block size boundary. > > To avoid using blkdev_issue_zeroout() in this path, I proposed another > patch here which adds another dax operation to zero out a rangex and > clear poison. > > https://lore.kernel.org/linux-fsdevel/20200123165249.GA7664@redhat.com/ > > Thinking more about it, it might be an overkill to solve this problem. > > How about if we just not clear poison/bad blocks when a partial page > is being zeroed. IOW, users will need to hole_punch whole filesystem > block worth of data which will free that block and it will be zeroed > some other time and clear poison in the process. For partial fs block > truncation/punch_hole, we don't clear poison. > > If above interface is acceptable, then we can get rid of code which > tries to call blkdev_issue_zeroout() in iomap_zero_range() path and > we don't have to implement another dax operation. > > Looking for some feedback on this. > > Vivek > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > fs/dax.c | 50 +++++++++++++++----------------------------------- > 1 file changed, 15 insertions(+), 35 deletions(-) > > Index: redhat-linux/fs/dax.c > =================================================================== > --- redhat-linux.orig/fs/dax.c 2020-01-29 15:19:18.551902448 -0500 > +++ redhat-linux/fs/dax.c 2020-01-29 15:40:56.584824549 -0500 > @@ -1044,47 +1044,27 @@ static vm_fault_t dax_load_hole(struct x > return ret; > } > > -static bool dax_range_is_aligned(struct block_device *bdev, > - unsigned int offset, unsigned int length) > -{ > - unsigned short sector_size = bdev_logical_block_size(bdev); > - > - if (!IS_ALIGNED(offset, sector_size)) > - return false; > - if (!IS_ALIGNED(length, sector_size)) > - return false; > - > - return true; > -} > - > int __dax_zero_page_range(struct block_device *bdev, > struct dax_device *dax_dev, sector_t sector, > unsigned int offset, unsigned int size) > { > - if (dax_range_is_aligned(bdev, offset, size)) { > - sector_t start_sector = sector + (offset >> 9); > - > - return blkdev_issue_zeroout(bdev, start_sector, > - size >> 9, GFP_NOFS, 0); > - } else { > - pgoff_t pgoff; > - long rc, id; > - void *kaddr; > - > - rc = bdev_dax_pgoff(bdev, sector, PAGE_SIZE, &pgoff); > - if (rc) > - return rc; > - > - id = dax_read_lock(); > - rc = dax_direct_access(dax_dev, pgoff, 1, &kaddr, NULL); > - if (rc < 0) { > - dax_read_unlock(id); > - return rc; > - } > - memset(kaddr + offset, 0, size); > - dax_flush(dax_dev, kaddr + offset, size); > + pgoff_t pgoff; > + long rc, id; > + void *kaddr; > + > + rc = bdev_dax_pgoff(bdev, sector, PAGE_SIZE, &pgoff); > + if (rc) > + return rc; > + > + id = dax_read_lock(); > + rc = dax_direct_access(dax_dev, pgoff, 1, &kaddr, NULL); > + if (rc < 0) { > dax_read_unlock(id); > + return rc; > } > + memset(kaddr + offset, 0, size); > + dax_flush(dax_dev, kaddr + offset, size); > + dax_read_unlock(id); > return 0; > } > EXPORT_SYMBOL_GPL(__dax_zero_page_range); > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org >
On Wed, Feb 5, 2020 at 12:27 PM <jane.chu@oracle.com> wrote: > > Hello, > > I haven't seen response to this proposal, unsure if there is a different > but related discussion ongoing... > > I'd like to express my wish: please make it easier for the pmem > applications when possible. > > If kernel does not clear poison when it could legitimately do so, The only path where this happens today is write() syscalls in dax mode, otherwise fallocate(PUNCH_HOLE) is currently the only guaranteed way to trigger error clearing from userspace (outside of sending raw commands to the device). > applications have to go through lengths to clear poisons. > For Cloud pmem applications that have upper bound on error recovery > time, not clearing poison while zeroing-out is quite undesirable. The complicating factor in all of this is the alignment requirement for clearing and the inability for native cpu instructions to clear errors. On current platforms talking to firmware is required and that interface may require 256-byte block clearing. This is why the implementation glommed on to clearing errors on block-I/O path writes because we at least knew that all of those I/Os were 512-byte aligned. This gets better with cpus that support the movdir64b instruction, in that case there is still a 64-byte alignment requirement, but there's no need to talk to the BIOS and therefore no need to talk to a driver. So we have this awkward dependency on block-device I/O semantics only because it happened to organize i/o in a way that supports error clearing. Right now the kernel does not install a pte on faults that land on a page with known poison, but only because the error clearing path is so convoluted and could only claim that fallocate(PUNCH_HOLE) cleared errors because that was guaranteed to send 512-byte aligned zero's down the block-I/O path when the fs-blocks got reallocated. In a world where native cpu instructions can clear errors the dax write() syscall case could be covered (modulo 64-byte alignment), and the kernel could just let the page be mapped so that the application could attempt it's own fine-grained clearing without calling back into the kernel.
Dan Williams <dan.j.williams@intel.com> writes: > Right now the kernel does not install a pte on faults that land on a > page with known poison, but only because the error clearing path is so > convoluted and could only claim that fallocate(PUNCH_HOLE) cleared > errors because that was guaranteed to send 512-byte aligned zero's > down the block-I/O path when the fs-blocks got reallocated. In a world > where native cpu instructions can clear errors the dax write() syscall > case could be covered (modulo 64-byte alignment), and the kernel could > just let the page be mapped so that the application could attempt it's > own fine-grained clearing without calling back into the kernel. I'm not sure we'd want to do allow mapping the PTEs even if there was support for clearing errors via CPU instructions. Any load from a poisoned page will result in an MCE, and there exists the possiblity that you will hit an unrecoverable error (Processor Context Corrupt). It's just safer to catch these cases by not mapping the page, and forcing recovery through the driver. -Jeff
On 2/18/20 11:50 AM, Jeff Moyer wrote: > Dan Williams <dan.j.williams@intel.com> writes: > >> Right now the kernel does not install a pte on faults that land on a >> page with known poison, but only because the error clearing path is so >> convoluted and could only claim that fallocate(PUNCH_HOLE) cleared >> errors because that was guaranteed to send 512-byte aligned zero's >> down the block-I/O path when the fs-blocks got reallocated. In a world >> where native cpu instructions can clear errors the dax write() syscall >> case could be covered (modulo 64-byte alignment), and the kernel could >> just let the page be mapped so that the application could attempt it's >> own fine-grained clearing without calling back into the kernel. > > I'm not sure we'd want to do allow mapping the PTEs even if there was > support for clearing errors via CPU instructions. Any load from a > poisoned page will result in an MCE, and there exists the possiblity > that you will hit an unrecoverable error (Processor Context Corrupt). > It's just safer to catch these cases by not mapping the page, and > forcing recovery through the driver. > > -Jeff > I'm still in the process of trying a number of things before making an attempt to respond to Dan's response. But I'm too slow, so I'd like to share some concerns I have here. If a poison in a file is consumed, and the signal handle does the repair and recover as follow: punch a hole the size at least 4K, then pwrite the correct data in to the 'hole', then resume the operation. However, because the newly allocated pmem block (due to pwrite to the 'hole') is a different clean physical pmem block while the poisoned block remain unfixed, so we have a provisioning problem, because 1. DCPMEM is expensive hence there is likely little provision being provided by users; 2. lack up API between dax-filesystem and pmem driver for clearing poison at each legitimate point, such as when the filesystem tries to allocate a pmem block, or zeroing out a range. As DCPMM is used for its performance and capacity in cloud application, which translates to that the performance code paths include the error handling and recovery code path... With respect to the new cpu instruction, my concern is about the API including the error blast radius as reported in the signal payload. Is there a venue where we could discuss more in detail ? Regards, -jane
On 2/18/20 12:45 PM, jane.chu@oracle.com wrote: > On 2/18/20 11:50 AM, Jeff Moyer wrote: >> Dan Williams <dan.j.williams@intel.com> writes: >> >>> Right now the kernel does not install a pte on faults that land on a >>> page with known poison, but only because the error clearing path is so >>> convoluted and could only claim that fallocate(PUNCH_HOLE) cleared >>> errors because that was guaranteed to send 512-byte aligned zero's >>> down the block-I/O path when the fs-blocks got reallocated. In a world >>> where native cpu instructions can clear errors the dax write() syscall >>> case could be covered (modulo 64-byte alignment), and the kernel could >>> just let the page be mapped so that the application could attempt it's >>> own fine-grained clearing without calling back into the kernel. >> >> I'm not sure we'd want to do allow mapping the PTEs even if there was >> support for clearing errors via CPU instructions. Any load from a >> poisoned page will result in an MCE, and there exists the possiblity >> that you will hit an unrecoverable error (Processor Context Corrupt). >> It's just safer to catch these cases by not mapping the page, and >> forcing recovery through the driver. >> >> -Jeff >> > > I'm still in the process of trying a number of things before making an > attempt to respond to Dan's response. But I'm too slow, so I'd like > to share some concerns I have here. > > If a poison in a file is consumed, and the signal handle does the > repair and recover as follow: punch a hole the size at least 4K, then > pwrite the correct data in to the 'hole', then resume the operation. > However, because the newly allocated pmem block (due to pwrite to the > 'hole') is a different clean physical pmem block while the poisoned > block remain unfixed, so we have a provisioning problem, because > 1. DCPMEM is expensive hence there is likely little provision being > provided by users; > 2. lack up API between dax-filesystem and pmem driver for clearing > poison at each legitimate point, such as when the filesystem tries > to allocate a pmem block, or zeroing out a range > > As DCPMM is used for its performance and capacity in cloud application, > which translates to that the performance code paths include the error > handling and recovery code path... > > With respect to the new cpu instruction, my concern is about the API > including the error blast radius as reported in the signal payload. > Is there a venue where we could discuss more in detail ? For all the quarantined poison blocks, it's not practical to clear them poisons via ndctl/libndctl on a per namespace granularity for fear of poisons occurred in valid pmem blocks during data at rest. How to ultimately clear poisons in a dax-fs in current framework? it seems to me poisons need to be cleared on the go automatically. Regards, -jane > > Regards, > -jane > > >
Index: redhat-linux/fs/dax.c =================================================================== --- redhat-linux.orig/fs/dax.c 2020-01-29 15:19:18.551902448 -0500 +++ redhat-linux/fs/dax.c 2020-01-29 15:40:56.584824549 -0500 @@ -1044,47 +1044,27 @@ static vm_fault_t dax_load_hole(struct x return ret; } -static bool dax_range_is_aligned(struct block_device *bdev, - unsigned int offset, unsigned int length) -{ - unsigned short sector_size = bdev_logical_block_size(bdev); - - if (!IS_ALIGNED(offset, sector_size)) - return false; - if (!IS_ALIGNED(length, sector_size)) - return false; - - return true; -} - int __dax_zero_page_range(struct block_device *bdev, struct dax_device *dax_dev, sector_t sector, unsigned int offset, unsigned int size) { - if (dax_range_is_aligned(bdev, offset, size)) { - sector_t start_sector = sector + (offset >> 9); - - return blkdev_issue_zeroout(bdev, start_sector, - size >> 9, GFP_NOFS, 0); - } else { - pgoff_t pgoff; - long rc, id; - void *kaddr; - - rc = bdev_dax_pgoff(bdev, sector, PAGE_SIZE, &pgoff); - if (rc) - return rc; - - id = dax_read_lock(); - rc = dax_direct_access(dax_dev, pgoff, 1, &kaddr, NULL); - if (rc < 0) { - dax_read_unlock(id); - return rc; - } - memset(kaddr + offset, 0, size); - dax_flush(dax_dev, kaddr + offset, size); + pgoff_t pgoff; + long rc, id; + void *kaddr; + + rc = bdev_dax_pgoff(bdev, sector, PAGE_SIZE, &pgoff); + if (rc) + return rc; + + id = dax_read_lock(); + rc = dax_direct_access(dax_dev, pgoff, 1, &kaddr, NULL); + if (rc < 0) { dax_read_unlock(id); + return rc; } + memset(kaddr + offset, 0, size); + dax_flush(dax_dev, kaddr + offset, size); + dax_read_unlock(id); return 0; } EXPORT_SYMBOL_GPL(__dax_zero_page_range);
I am looking into getting rid of dependency on block device in dax path. One such place is __dax_zero_page_range() which checks if range being zeroed is aligned to block device logical size, then it calls bdev_issue_zeroout() instead of doing memset(). Calling blkdev_issue_zeroout() also clears bad blocks and poison if any in that range. This path is used by iomap_zero_range() which in-turn is being used by filesystems to zero partial filesystem system blocks. For zeroing full filesystem blocks we seem to be calling blkdev_issue_zeroout() which clears bad blocks and poison in that range. So this code currently only seems to help with partial filesystem block zeroing. That too only if truncation/hole_punch happens on device logical block size boundary. To avoid using blkdev_issue_zeroout() in this path, I proposed another patch here which adds another dax operation to zero out a rangex and clear poison. https://lore.kernel.org/linux-fsdevel/20200123165249.GA7664@redhat.com/ Thinking more about it, it might be an overkill to solve this problem. How about if we just not clear poison/bad blocks when a partial page is being zeroed. IOW, users will need to hole_punch whole filesystem block worth of data which will free that block and it will be zeroed some other time and clear poison in the process. For partial fs block truncation/punch_hole, we don't clear poison. If above interface is acceptable, then we can get rid of code which tries to call blkdev_issue_zeroout() in iomap_zero_range() path and we don't have to implement another dax operation. Looking for some feedback on this. Vivek Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/dax.c | 50 +++++++++++++++----------------------------------- 1 file changed, 15 insertions(+), 35 deletions(-)