From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965103AbbKCWuf (ORCPT ); Tue, 3 Nov 2015 17:50:35 -0500 Received: from mail-wi0-f170.google.com ([209.85.212.170]:34464 "EHLO mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965023AbbKCWue (ORCPT ); Tue, 3 Nov 2015 17:50:34 -0500 MIME-Version: 1.0 In-Reply-To: <20151103190140.GB23366@linux.intel.com> References: <20151102042941.6610.27784.stgit@dwillia2-desk3.amr.corp.intel.com> <20151102042958.6610.65193.stgit@dwillia2-desk3.amr.corp.intel.com> <20151103190140.GB23366@linux.intel.com> Date: Tue, 3 Nov 2015 14:50:32 -0800 Message-ID: Subject: Re: [PATCH v3 03/15] block, dax: fix lifetime of in-kernel dax mappings with dax_map_atomic() From: Dan Williams To: Ross Zwisler Cc: Jens Axboe , Jens Axboe , Jan Kara , "linux-nvdimm@lists.01.org" , david , "linux-kernel@vger.kernel.org" , Jeff Moyer , Jan Kara , Christoph Hellwig Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 3, 2015 at 11:01 AM, Ross Zwisler wrote: > On Sun, Nov 01, 2015 at 11:29:58PM -0500, Dan Williams wrote: >> The DAX implementation needs to protect new calls to ->direct_access() >> and usage of its return value against unbind of the underlying block >> device. Use blk_queue_enter()/blk_queue_exit() to either prevent >> blk_cleanup_queue() from proceeding, or fail the dax_map_atomic() if the >> request_queue is being torn down. >> >> Cc: Jan Kara >> Cc: Jens Axboe >> Cc: Christoph Hellwig >> Cc: Dave Chinner >> Cc: Ross Zwisler >> Reviewed-by: Jeff Moyer >> Signed-off-by: Dan Williams >> --- > <> [ trim the comments that Jeff responded to] >> @@ -305,11 +353,10 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh, >> goto out; >> } >> >> - error = bdev_direct_access(bh->b_bdev, sector, &addr, &pfn, bh->b_size); >> - if (error < 0) >> - goto out; >> - if (error < PAGE_SIZE) { >> - error = -EIO; >> + addr = __dax_map_atomic(bdev, to_sector(bh, inode), bh->b_size, >> + &pfn, NULL); >> + if (IS_ERR(addr)) { >> + error = PTR_ERR(addr); > > Just a note that we lost the check for bdev_direct_access() returning less > than PAGE_SIZE. Are we sure this can't happen and that it's safe to remove > the check? Yes, as Jeff recommends I'll do a follow on patch to make this an explicit guarantee of bdev_direct_access() just like the page alignment. > >> @@ -609,15 +655,20 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address, >> result = VM_FAULT_NOPAGE; >> spin_unlock(ptl); >> } else { >> - sector = bh.b_blocknr << (blkbits - 9); >> - length = bdev_direct_access(bh.b_bdev, sector, &kaddr, &pfn, >> - bh.b_size); >> - if (length < 0) { >> + long length; >> + unsigned long pfn; >> + void __pmem *kaddr = __dax_map_atomic(bdev, >> + to_sector(&bh, inode), HPAGE_SIZE, &pfn, >> + &length); > > Let's use PMD_SIZE instead of HPAGE_SIZE to be consistent with the rest of the > DAX code. > I changed to HPAGE_SIZE on advice from Dave Hansen. I'll insert a preceding cleanup patch in this series to do the conversion since we should be consistent with the use of PAGE_SIZE in the other dax paths. >> + >> + if (IS_ERR(kaddr)) { >> result = VM_FAULT_SIGBUS; >> goto out; >> } >> - if ((length < PMD_SIZE) || (pfn & PG_PMD_COLOUR)) >> + if ((length < PMD_SIZE) || (pfn & PG_PMD_COLOUR)) { >> + dax_unmap_atomic(bdev, kaddr); >> goto fallback; >> + } >> >> if (buffer_unwritten(&bh) || buffer_new(&bh)) { >> clear_pmem(kaddr, HPAGE_SIZE); > > Ditto, let's use PMD_SIZE for consistency (I realize this was changed ealier > in the series). Ditto on the rebuttal.