| Message ID | 148521639006.25631.4184774582309071957.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 23, 2017 at 05:06:30PM -0700, Dave Jiang wrote: > __GFP_IO got accidentally dropped in the dax fault handler during the > vmf parameter conversion. Putting it back. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> I don't see why this is needed? We already have an overly generous gfp_mask that explicitly includes both __GFP_FS (which is probably wrong), and __GFP_IO. I ran a quick test to verify this, and when I get a PMD fault my mask does indeed include both __GFP_IO and __GFP_FS. The test was done with the current mmots/master and this patch. The mask is set up in __handle_mm_fault() => __get_fault_gfp_mask(). So, no need to mess with the mask. Also, the patch is wrong because if __GFP_IO *was* already part of the gfp_mask, this patch would re-add it, then unconditionally remove it. So, with the current code we would end up with a more restrictive mask when we exited the PMD fault handler. Really if we wanted to do something like this we would need to save and restore, not just OR in a value and then mask it back out. > --- > fs/dax.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/dax.c b/fs/dax.c > index 2e90f7a..20e9db8 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1332,6 +1332,7 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf, struct iomap_ops *ops) > loff_t pos; > int error; > > + vmf->gfp_mask |= __GFP_IO; > /* > * Check whether offset isn't beyond end of file now. Caller is > * supposed to hold locks serializing us with truncate / punch hole so > @@ -1423,6 +1424,7 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf, struct iomap_ops *ops) > } > out: > trace_dax_pmd_fault_done(inode, vmf, max_pgoff, result); > + vmf->gfp_mask &= ~__GFP_IO; > return result; > } > #else >
> -----Original Message----- > From: Ross Zwisler [mailto:ross.zwisler@linux.intel.com] > Sent: Tuesday, January 24, 2017 11:32 AM > To: Jiang, Dave <dave.jiang@intel.com> > Cc: Williams, Dan J <dan.j.williams@intel.com>; ross.zwisler@linux.intel.com; linux-nvdimm@lists.01.org > Subject: Re: [PATCH] dax: put back __GFP_IO in the dax fault handler > > On Mon, Jan 23, 2017 at 05:06:30PM -0700, Dave Jiang wrote: > > __GFP_IO got accidentally dropped in the dax fault handler during the > > vmf parameter conversion. Putting it back. > > > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > I don't see why this is needed? We already have an overly generous gfp_mask > that explicitly includes both __GFP_FS (which is probably wrong), and > __GFP_IO. I ran a quick test to verify this, and when I get a PMD fault my > mask does indeed include both __GFP_IO and __GFP_FS. The test was done with > the current mmots/master and this patch. > > The mask is set up in __handle_mm_fault() => __get_fault_gfp_mask(). > > So, no need to mess with the mask. Also, the patch is wrong because if > __GFP_IO *was* already part of the gfp_mask, this patch would re-add it, then > unconditionally remove it. So, with the current code we would end up with a > more restrictive mask when we exited the PMD fault handler. Really if we > wanted to do something like this we would need to save and restore, not just > OR in a value and then mask it back out. Ok I'll drop the patch. > > > --- > > fs/dax.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/dax.c b/fs/dax.c > > index 2e90f7a..20e9db8 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -1332,6 +1332,7 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf, struct iomap_ops *ops) > > loff_t pos; > > int error; > > > > + vmf->gfp_mask |= __GFP_IO; > > /* > > * Check whether offset isn't beyond end of file now. Caller is > > * supposed to hold locks serializing us with truncate / punch hole so > > @@ -1423,6 +1424,7 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf, struct iomap_ops *ops) > > } > > out: > > trace_dax_pmd_fault_done(inode, vmf, max_pgoff, result); > > + vmf->gfp_mask &= ~__GFP_IO; > > return result; > > } > > #else > >
diff --git a/fs/dax.c b/fs/dax.c index 2e90f7a..20e9db8 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -1332,6 +1332,7 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf, struct iomap_ops *ops) loff_t pos; int error; + vmf->gfp_mask |= __GFP_IO; /* * Check whether offset isn't beyond end of file now. Caller is * supposed to hold locks serializing us with truncate / punch hole so @@ -1423,6 +1424,7 @@ static int dax_iomap_pmd_fault(struct vm_fault *vmf, struct iomap_ops *ops) } out: trace_dax_pmd_fault_done(inode, vmf, max_pgoff, result); + vmf->gfp_mask &= ~__GFP_IO; return result; } #else
__GFP_IO got accidentally dropped in the dax fault handler during the vmf parameter conversion. Putting it back. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- fs/dax.c | 2 ++ 1 file changed, 2 insertions(+)