* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map [not found] <4ac1b013-6193-da70-3384-d8878a247fbc@mykernel.net> @ 2020-08-30 7:10 ` Chen, Rong A 2020-09-10 0:39 ` Rong Chen 1 sibling, 0 replies; 12+ messages in thread From: Chen, Rong A @ 2020-08-30 7:10 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 8090 bytes --] On 8/29/2020 8:28 PM, cgxu wrote: > On 8/29/20 7:35 PM, kernel test robot wrote: >> Hi Chengguang, >> >> [FYI, it's a private test report for your RFC patch.] >> [auto build test ERROR on miklos-vfs/overlayfs-next] >> [also build test ERROR on v5.9-rc2 next-20200828] >> [cannot apply to hnaz-linux-mm/master] >> [If your patch is applied to the wrong git tree, kindly drop us a note. >> And when submitting patch, we suggest to use '--base' as documented in >> https://git-scm.com/docs/git-format-patch] >> >> url: >> https://github.com/0day-ci/linux/commits/Chengguang-Xu/ovl-stacked-mmap-for-shared-map/20200829-175321 >> base: >> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git >> overlayfs-next >> config: sh-allmodconfig (attached as .config) >> compiler: sh4-linux-gcc (GCC) 9.3.0 >> reproduce (this is a W=1 build): >> wget >> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross >> -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # save the attached .config to linux build tree >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 >> make.cross ARCH=sh >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot <lkp@intel.com> > > It seems missed applying two previous patches in the patch set, > actually, the patch set is based on miklos-vfs/overlayfs-next. Hi Chengguang, Thanks for the feedback, we'll take a look. Best Regards, Rong Chen > > cgxu > >> >> All error/warnings (new ones prefixed by >>): >> >> fs/overlayfs/file.c:37:14: warning: no previous prototype for >> 'ovl_get_realfile' [-Wmissing-prototypes] >> 37 | struct file *ovl_get_realfile(struct file *file) >> | ^~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c:44:6: warning: no previous prototype for >> 'ovl_set_realfile' [-Wmissing-prototypes] >> 44 | void ovl_set_realfile(struct file *file, struct file >> *realfile) >> | ^~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c:51:36: warning: no previous prototype for >> 'ovl_get_real_vmops' [-Wmissing-prototypes] >> 51 | const struct vm_operations_struct >> *ovl_get_real_vmops(struct file *file) >> | ^~~~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c:58:6: warning: no previous prototype for >> 'ovl_set_real_vmops' [-Wmissing-prototypes] >> 58 | void ovl_set_real_vmops(struct file *file, >> | ^~~~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c: In function 'ovl_fault': >>>> fs/overlayfs/file.c:518:6: error: implicit declaration of function >>>> 'fault_flag_check' [-Werror=implicit-function-declaration] >> 518 | if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { >> | ^~~~~~~~~~~~~~~~ >>>> fs/overlayfs/file.c:551:10: error: implicit declaration of function >>>> 'maybe_unlock_mmap_for_io' [-Werror=implicit-function-declaration] >> 551 | fpin = maybe_unlock_mmap_for_io(vmf, NULL); >> | ^~~~~~~~~~~~~~~~~~~~~~~~ >>>> fs/overlayfs/file.c:551:8: warning: assignment to 'struct file *' >>>> from 'int' makes pointer from integer without a cast >>>> [-Wint-conversion] >> 551 | fpin = maybe_unlock_mmap_for_io(vmf, NULL); >> | ^ >> cc1: some warnings being treated as errors >> >> # >> https://github.com/0day-ci/linux/commit/a33b1899ccf797aa55f56333a2700cfbc67487cc >> git remote add linux-review https://github.com/0day-ci/linux >> git fetch --no-tags linux-review >> Chengguang-Xu/ovl-stacked-mmap-for-shared-map/20200829-175321 >> git checkout a33b1899ccf797aa55f56333a2700cfbc67487cc >> vim +/fault_flag_check +518 fs/overlayfs/file.c >> >> 504 >> 505 vm_fault_t ovl_fault(struct vm_fault *vmf) >> 506 { >> 507 struct vm_area_struct *vma = vmf->vma; >> 508 struct file *file = vma->vm_file; >> 509 struct file *realfile; >> 510 struct file *fpin, *tmp; >> 511 struct inode *inode = file_inode(file); >> 512 struct inode *realinode; >> 513 const struct cred *old_cred; >> 514 bool retry_allowed; >> 515 vm_fault_t ret; >> 516 int err = 0; >> 517 >> > 518 if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { >> 519 realfile = ovl_get_realfile(file); >> 520 >> 521 if (!ovl_has_upperdata(inode) || >> 522 realfile->f_inode != ovl_inode_upper(inode) || >> 523 !realfile->f_op->mmap) >> 524 return VM_FAULT_SIGBUS; >> 525 >> 526 if (!ovl_get_real_vmops(file)) { >> 527 old_cred = ovl_override_creds(inode->i_sb); >> 528 err = call_mmap(realfile, vma); >> 529 revert_creds(old_cred); >> 530 >> 531 vma->vm_file = file; >> 532 if (err) { >> 533 vma->vm_ops = &ovl_vm_ops; >> 534 return VM_FAULT_SIGBUS; >> 535 } >> 536 ovl_set_real_vmops(file, vma->vm_ops); >> 537 vma->vm_ops = &ovl_vm_ops; >> 538 } >> 539 >> 540 retry_allowed = fault_flag_check(vmf, >> FAULT_FLAG_ALLOW_RETRY); >> 541 if (retry_allowed) >> 542 vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; >> 543 vma->vm_file = realfile; >> 544 ret = ovl_get_real_vmops(file)->fault(vmf); >> 545 vma->vm_file = file; >> 546 if (retry_allowed) >> 547 vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; >> 548 return ret; >> 549 >> 550 } else { >> > 551 fpin = maybe_unlock_mmap_for_io(vmf, NULL); >> 552 if (!fpin) >> 553 return VM_FAULT_SIGBUS; >> 554 >> 555 ret = VM_FAULT_RETRY; >> 556 if (!ovl_has_upperdata(inode)) { >> 557 err = ovl_copy_up_with_data(file->f_path.dentry); >> 558 if (err) >> 559 goto out; >> 560 } >> 561 >> 562 realinode = ovl_inode_realdata(inode); >> 563 realfile = ovl_open_realfile(file, realinode); >> 564 if (IS_ERR(realfile)) >> 565 goto out; >> 566 >> 567 tmp = ovl_get_realfile(file); >> 568 ovl_set_realfile(file, realfile); >> 569 fput(tmp); >> 570 >> 571 out: >> 572 fput(fpin); >> 573 return ret; >> 574 } >> 575 } >> 576 >> >> --- >> 0-DAY CI Kernel Test Service, Intel Corporation >> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map [not found] <4ac1b013-6193-da70-3384-d8878a247fbc@mykernel.net> 2020-08-30 7:10 ` [RFC PATCH 3/3] ovl: implement stacked mmap for shared map Chen, Rong A @ 2020-09-10 0:39 ` Rong Chen 1 sibling, 0 replies; 12+ messages in thread From: Rong Chen @ 2020-09-10 0:39 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 8484 bytes --] On 8/29/20 8:28 PM, cgxu wrote: > On 8/29/20 7:35 PM, kernel test robot wrote: >> Hi Chengguang, >> >> [FYI, it's a private test report for your RFC patch.] >> [auto build test ERROR on miklos-vfs/overlayfs-next] >> [also build test ERROR on v5.9-rc2 next-20200828] >> [cannot apply to hnaz-linux-mm/master] >> [If your patch is applied to the wrong git tree, kindly drop us a note. >> And when submitting patch, we suggest to use '--base' as documented in >> https://git-scm.com/docs/git-format-patch] >> >> url: >> https://github.com/0day-ci/linux/commits/Chengguang-Xu/ovl-stacked-mmap-for-shared-map/20200829-175321 >> base: >> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git >> overlayfs-next >> config: sh-allmodconfig (attached as .config) >> compiler: sh4-linux-gcc (GCC) 9.3.0 >> reproduce (this is a W=1 build): >> wget >> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross >> -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # save the attached .config to linux build tree >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 >> make.cross ARCH=sh >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot <lkp@intel.com> > > It seems missed applying two previous patches in the patch set, > actually, the patch set is based on miklos-vfs/overlayfs-next. Hi cgxu, The bot applied the patch set based on commit 4518dfcf761e3 which can be also found in miklos-vfs/overlayfs-next, is the base commit not correct? or could you provide the base commit you used. a33b1899ccf79 ovl: implement stacked mmap for shared map af28de469a59c ovl: introduce struct ovl_file_entry 3229f1dd50299 mm: mmap: export necessary functions for overlayfs' mmap 4518dfcf761e3 ovl: fix lookup of indexed hardlinks with metacopy Best Regards, Rong Chen > > cgxu > >> >> All error/warnings (new ones prefixed by >>): >> >> fs/overlayfs/file.c:37:14: warning: no previous prototype for >> 'ovl_get_realfile' [-Wmissing-prototypes] >> 37 | struct file *ovl_get_realfile(struct file *file) >> | ^~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c:44:6: warning: no previous prototype for >> 'ovl_set_realfile' [-Wmissing-prototypes] >> 44 | void ovl_set_realfile(struct file *file, struct file >> *realfile) >> | ^~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c:51:36: warning: no previous prototype for >> 'ovl_get_real_vmops' [-Wmissing-prototypes] >> 51 | const struct vm_operations_struct >> *ovl_get_real_vmops(struct file *file) >> | ^~~~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c:58:6: warning: no previous prototype for >> 'ovl_set_real_vmops' [-Wmissing-prototypes] >> 58 | void ovl_set_real_vmops(struct file *file, >> | ^~~~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c: In function 'ovl_fault': >>>> fs/overlayfs/file.c:518:6: error: implicit declaration of function >>>> 'fault_flag_check' [-Werror=implicit-function-declaration] >> 518 | if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { >> | ^~~~~~~~~~~~~~~~ >>>> fs/overlayfs/file.c:551:10: error: implicit declaration of function >>>> 'maybe_unlock_mmap_for_io' [-Werror=implicit-function-declaration] >> 551 | fpin = maybe_unlock_mmap_for_io(vmf, NULL); >> | ^~~~~~~~~~~~~~~~~~~~~~~~ >>>> fs/overlayfs/file.c:551:8: warning: assignment to 'struct file *' >>>> from 'int' makes pointer from integer without a cast >>>> [-Wint-conversion] >> 551 | fpin = maybe_unlock_mmap_for_io(vmf, NULL); >> | ^ >> cc1: some warnings being treated as errors >> >> # >> https://github.com/0day-ci/linux/commit/a33b1899ccf797aa55f56333a2700cfbc67487cc >> git remote add linux-review https://github.com/0day-ci/linux >> git fetch --no-tags linux-review >> Chengguang-Xu/ovl-stacked-mmap-for-shared-map/20200829-175321 >> git checkout a33b1899ccf797aa55f56333a2700cfbc67487cc >> vim +/fault_flag_check +518 fs/overlayfs/file.c >> >> 504 >> 505 vm_fault_t ovl_fault(struct vm_fault *vmf) >> 506 { >> 507 struct vm_area_struct *vma = vmf->vma; >> 508 struct file *file = vma->vm_file; >> 509 struct file *realfile; >> 510 struct file *fpin, *tmp; >> 511 struct inode *inode = file_inode(file); >> 512 struct inode *realinode; >> 513 const struct cred *old_cred; >> 514 bool retry_allowed; >> 515 vm_fault_t ret; >> 516 int err = 0; >> 517 >> > 518 if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { >> 519 realfile = ovl_get_realfile(file); >> 520 >> 521 if (!ovl_has_upperdata(inode) || >> 522 realfile->f_inode != ovl_inode_upper(inode) || >> 523 !realfile->f_op->mmap) >> 524 return VM_FAULT_SIGBUS; >> 525 >> 526 if (!ovl_get_real_vmops(file)) { >> 527 old_cred = ovl_override_creds(inode->i_sb); >> 528 err = call_mmap(realfile, vma); >> 529 revert_creds(old_cred); >> 530 >> 531 vma->vm_file = file; >> 532 if (err) { >> 533 vma->vm_ops = &ovl_vm_ops; >> 534 return VM_FAULT_SIGBUS; >> 535 } >> 536 ovl_set_real_vmops(file, vma->vm_ops); >> 537 vma->vm_ops = &ovl_vm_ops; >> 538 } >> 539 >> 540 retry_allowed = fault_flag_check(vmf, >> FAULT_FLAG_ALLOW_RETRY); >> 541 if (retry_allowed) >> 542 vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; >> 543 vma->vm_file = realfile; >> 544 ret = ovl_get_real_vmops(file)->fault(vmf); >> 545 vma->vm_file = file; >> 546 if (retry_allowed) >> 547 vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; >> 548 return ret; >> 549 >> 550 } else { >> > 551 fpin = maybe_unlock_mmap_for_io(vmf, NULL); >> 552 if (!fpin) >> 553 return VM_FAULT_SIGBUS; >> 554 >> 555 ret = VM_FAULT_RETRY; >> 556 if (!ovl_has_upperdata(inode)) { >> 557 err = ovl_copy_up_with_data(file->f_path.dentry); >> 558 if (err) >> 559 goto out; >> 560 } >> 561 >> 562 realinode = ovl_inode_realdata(inode); >> 563 realfile = ovl_open_realfile(file, realinode); >> 564 if (IS_ERR(realfile)) >> 565 goto out; >> 566 >> 567 tmp = ovl_get_realfile(file); >> 568 ovl_set_realfile(file, realfile); >> 569 fput(tmp); >> 570 >> 571 out: >> 572 fput(fpin); >> 573 return ret; >> 574 } >> 575 } >> 576 >> >> --- >> 0-DAY CI Kernel Test Service, Intel Corporation >> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 0/3] ovl: stacked mmap for shared map @ 2020-08-29 9:50 Chengguang Xu 2020-08-29 9:51 ` [RFC PATCH 3/3] ovl: implement " Chengguang Xu 0 siblings, 1 reply; 12+ messages in thread From: Chengguang Xu @ 2020-08-29 9:50 UTC (permalink / raw) To: linux-unionfs, linux-mm; +Cc: miklos, akpm, amir73il, riteshh, Chengguang Xu Currently, there is still ro/rw inconsistency related to shared mmap in overlayfs, this patch set implements stacked mmap for shared map and transfer necessary operations to upper inode, so that we can keep data consistency in any kind of mmap. Patch 1 exports necessary functions from kernel to module. Patch 2 introduces struct ovl_file_entry to store real vm_ops. Patch 3 implements stacked mmap for shared map to keep data consistency. Chengguang Xu (3): mm: mmap: export necessary functions for overlayfs' mmap ovl: introduce struct ovl_file_entry ovl: implement stacked mmap for shared map fs/overlayfs/file.c | 178 ++++++++++++++++++++++++++++++++++++++++---- include/linux/mm.h | 2 + mm/filemap.c | 28 +++++++ mm/internal.h | 22 ------ 4 files changed, 195 insertions(+), 35 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 3/3] ovl: implement stacked mmap for shared map 2020-08-29 9:50 [RFC PATCH 0/3] ovl: " Chengguang Xu @ 2020-08-29 9:51 ` Chengguang Xu 2020-08-29 11:35 ` kernel test robot 2020-08-30 11:33 ` Amir Goldstein 0 siblings, 2 replies; 12+ messages in thread From: Chengguang Xu @ 2020-08-29 9:51 UTC (permalink / raw) To: linux-unionfs, linux-mm; +Cc: miklos, akpm, amir73il, riteshh, Chengguang Xu Implement stacked mmap for shared map to keep data consistency. Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> --- fs/overlayfs/file.c | 120 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 114 insertions(+), 6 deletions(-) diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 14ab5344a918..db5ab200d984 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -21,9 +21,17 @@ struct ovl_aio_req { struct fd fd; }; +static vm_fault_t ovl_fault(struct vm_fault *vmf); +static vm_fault_t ovl_page_mkwrite(struct vm_fault *vmf); + +static const struct vm_operations_struct ovl_vm_ops = { + .fault = ovl_fault, + .page_mkwrite = ovl_page_mkwrite, +}; + struct ovl_file_entry { struct file *realfile; - void *vm_ops; + const struct vm_operations_struct *vm_ops; }; struct file *ovl_get_realfile(struct file *file) @@ -40,14 +48,15 @@ void ovl_set_realfile(struct file *file, struct file *realfile) ofe->realfile = realfile; } -void *ovl_get_real_vmops(struct file *file) +const struct vm_operations_struct *ovl_get_real_vmops(struct file *file) { struct ovl_file_entry *ofe = file->private_data; return ofe->vm_ops; } -void ovl_set_real_vmops(struct file *file, void *vm_ops) +void ovl_set_real_vmops(struct file *file, + const struct vm_operations_struct *vm_ops) { struct ovl_file_entry *ofe = file->private_data; @@ -493,11 +502,104 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) return ret; } +vm_fault_t ovl_fault(struct vm_fault *vmf) +{ + struct vm_area_struct *vma = vmf->vma; + struct file *file = vma->vm_file; + struct file *realfile; + struct file *fpin, *tmp; + struct inode *inode = file_inode(file); + struct inode *realinode; + const struct cred *old_cred; + bool retry_allowed; + vm_fault_t ret; + int err = 0; + + if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { + realfile = ovl_get_realfile(file); + + if (!ovl_has_upperdata(inode) || + realfile->f_inode != ovl_inode_upper(inode) || + !realfile->f_op->mmap) + return VM_FAULT_SIGBUS; + + if (!ovl_get_real_vmops(file)) { + old_cred = ovl_override_creds(inode->i_sb); + err = call_mmap(realfile, vma); + revert_creds(old_cred); + + vma->vm_file = file; + if (err) { + vma->vm_ops = &ovl_vm_ops; + return VM_FAULT_SIGBUS; + } + ovl_set_real_vmops(file, vma->vm_ops); + vma->vm_ops = &ovl_vm_ops; + } + + retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); + if (retry_allowed) + vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; + vma->vm_file = realfile; + ret = ovl_get_real_vmops(file)->fault(vmf); + vma->vm_file = file; + if (retry_allowed) + vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; + return ret; + + } else { + fpin = maybe_unlock_mmap_for_io(vmf, NULL); + if (!fpin) + return VM_FAULT_SIGBUS; + + ret = VM_FAULT_RETRY; + if (!ovl_has_upperdata(inode)) { + err = ovl_copy_up_with_data(file->f_path.dentry); + if (err) + goto out; + } + + realinode = ovl_inode_realdata(inode); + realfile = ovl_open_realfile(file, realinode); + if (IS_ERR(realfile)) + goto out; + + tmp = ovl_get_realfile(file); + ovl_set_realfile(file, realfile); + fput(tmp); + +out: + fput(fpin); + return ret; + } +} + +static vm_fault_t ovl_page_mkwrite(struct vm_fault *vmf) +{ + struct vm_area_struct *vma = vmf->vma; + struct file *file = vma->vm_file; + struct file *realfile; + struct inode *inode = file_inode(file); + vm_fault_t ret; + + realfile = ovl_get_realfile(file); + + sb_start_pagefault(inode->i_sb); + file_update_time(file); + + vma->vm_file = realfile; + ret = ovl_get_real_vmops(file)->page_mkwrite(vmf); + vma->vm_file = file; + + sb_end_pagefault(inode->i_sb); + return ret; +} + static int ovl_mmap(struct file *file, struct vm_area_struct *vma) { struct file *realfile = ovl_get_realfile(file); const struct cred *old_cred; - int ret; + int ret = 0; if (!realfile->f_op->mmap) return -ENODEV; @@ -505,6 +607,13 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma) if (WARN_ON(file != vma->vm_file)) return -EIO; + if (!ovl_has_upperdata(file_inode(file)) && + (vma->vm_flags & (VM_SHARED|VM_MAYSHARE))) { + vma->vm_ops = &ovl_vm_ops; + ovl_file_accessed(file); + return 0; + } + vma->vm_file = get_file(realfile); old_cred = ovl_override_creds(file_inode(file)->i_sb); @@ -517,10 +626,9 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma) } else { /* Drop reference count from previous vm_file value */ fput(file); + ovl_file_accessed(file); } - ovl_file_accessed(file); - return ret; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map 2020-08-29 9:51 ` [RFC PATCH 3/3] ovl: implement " Chengguang Xu @ 2020-08-29 11:35 ` kernel test robot 2020-08-30 11:33 ` Amir Goldstein 1 sibling, 0 replies; 12+ messages in thread From: kernel test robot @ 2020-08-29 11:35 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 5572 bytes --] Hi Chengguang, [FYI, it's a private test report for your RFC patch.] [auto build test ERROR on miklos-vfs/overlayfs-next] [also build test ERROR on v5.9-rc2 next-20200828] [cannot apply to hnaz-linux-mm/master] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Chengguang-Xu/ovl-stacked-mmap-for-shared-map/20200829-175321 base: https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next config: sh-allmodconfig (attached as .config) compiler: sh4-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All error/warnings (new ones prefixed by >>): fs/overlayfs/file.c:37:14: warning: no previous prototype for 'ovl_get_realfile' [-Wmissing-prototypes] 37 | struct file *ovl_get_realfile(struct file *file) | ^~~~~~~~~~~~~~~~ fs/overlayfs/file.c:44:6: warning: no previous prototype for 'ovl_set_realfile' [-Wmissing-prototypes] 44 | void ovl_set_realfile(struct file *file, struct file *realfile) | ^~~~~~~~~~~~~~~~ fs/overlayfs/file.c:51:36: warning: no previous prototype for 'ovl_get_real_vmops' [-Wmissing-prototypes] 51 | const struct vm_operations_struct *ovl_get_real_vmops(struct file *file) | ^~~~~~~~~~~~~~~~~~ fs/overlayfs/file.c:58:6: warning: no previous prototype for 'ovl_set_real_vmops' [-Wmissing-prototypes] 58 | void ovl_set_real_vmops(struct file *file, | ^~~~~~~~~~~~~~~~~~ fs/overlayfs/file.c: In function 'ovl_fault': >> fs/overlayfs/file.c:518:6: error: implicit declaration of function 'fault_flag_check' [-Werror=implicit-function-declaration] 518 | if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { | ^~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c:551:10: error: implicit declaration of function 'maybe_unlock_mmap_for_io' [-Werror=implicit-function-declaration] 551 | fpin = maybe_unlock_mmap_for_io(vmf, NULL); | ^~~~~~~~~~~~~~~~~~~~~~~~ >> fs/overlayfs/file.c:551:8: warning: assignment to 'struct file *' from 'int' makes pointer from integer without a cast [-Wint-conversion] 551 | fpin = maybe_unlock_mmap_for_io(vmf, NULL); | ^ cc1: some warnings being treated as errors # https://github.com/0day-ci/linux/commit/a33b1899ccf797aa55f56333a2700cfbc67487cc git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Chengguang-Xu/ovl-stacked-mmap-for-shared-map/20200829-175321 git checkout a33b1899ccf797aa55f56333a2700cfbc67487cc vim +/fault_flag_check +518 fs/overlayfs/file.c 504 505 vm_fault_t ovl_fault(struct vm_fault *vmf) 506 { 507 struct vm_area_struct *vma = vmf->vma; 508 struct file *file = vma->vm_file; 509 struct file *realfile; 510 struct file *fpin, *tmp; 511 struct inode *inode = file_inode(file); 512 struct inode *realinode; 513 const struct cred *old_cred; 514 bool retry_allowed; 515 vm_fault_t ret; 516 int err = 0; 517 > 518 if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { 519 realfile = ovl_get_realfile(file); 520 521 if (!ovl_has_upperdata(inode) || 522 realfile->f_inode != ovl_inode_upper(inode) || 523 !realfile->f_op->mmap) 524 return VM_FAULT_SIGBUS; 525 526 if (!ovl_get_real_vmops(file)) { 527 old_cred = ovl_override_creds(inode->i_sb); 528 err = call_mmap(realfile, vma); 529 revert_creds(old_cred); 530 531 vma->vm_file = file; 532 if (err) { 533 vma->vm_ops = &ovl_vm_ops; 534 return VM_FAULT_SIGBUS; 535 } 536 ovl_set_real_vmops(file, vma->vm_ops); 537 vma->vm_ops = &ovl_vm_ops; 538 } 539 540 retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); 541 if (retry_allowed) 542 vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; 543 vma->vm_file = realfile; 544 ret = ovl_get_real_vmops(file)->fault(vmf); 545 vma->vm_file = file; 546 if (retry_allowed) 547 vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; 548 return ret; 549 550 } else { > 551 fpin = maybe_unlock_mmap_for_io(vmf, NULL); 552 if (!fpin) 553 return VM_FAULT_SIGBUS; 554 555 ret = VM_FAULT_RETRY; 556 if (!ovl_has_upperdata(inode)) { 557 err = ovl_copy_up_with_data(file->f_path.dentry); 558 if (err) 559 goto out; 560 } 561 562 realinode = ovl_inode_realdata(inode); 563 realfile = ovl_open_realfile(file, realinode); 564 if (IS_ERR(realfile)) 565 goto out; 566 567 tmp = ovl_get_realfile(file); 568 ovl_set_realfile(file, realfile); 569 fput(tmp); 570 571 out: 572 fput(fpin); 573 return ret; 574 } 575 } 576 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 55735 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map 2020-08-29 9:51 ` [RFC PATCH 3/3] ovl: implement " Chengguang Xu @ 2020-08-30 11:33 ` Amir Goldstein 2020-08-30 11:33 ` Amir Goldstein 1 sibling, 0 replies; 12+ messages in thread From: Amir Goldstein @ 2020-08-30 11:33 UTC (permalink / raw) To: Chengguang Xu Cc: overlayfs, Linux MM, Miklos Szeredi, Andrew Morton, Ritesh Harjani On Sat, Aug 29, 2020 at 12:51 PM Chengguang Xu <cgxu519@mykernel.net> wrote: > > Implement stacked mmap for shared map to keep data > consistency. > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> > --- > fs/overlayfs/file.c | 120 +++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 114 insertions(+), 6 deletions(-) > > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c > index 14ab5344a918..db5ab200d984 100644 > --- a/fs/overlayfs/file.c > +++ b/fs/overlayfs/file.c > @@ -21,9 +21,17 @@ struct ovl_aio_req { > struct fd fd; > }; > > +static vm_fault_t ovl_fault(struct vm_fault *vmf); > +static vm_fault_t ovl_page_mkwrite(struct vm_fault *vmf); > + > +static const struct vm_operations_struct ovl_vm_ops = { > + .fault = ovl_fault, > + .page_mkwrite = ovl_page_mkwrite, > +}; > + Interesting direction, not sure if this is workable. I don't know enough about mm to say. But what about the rest of the operations? Did you go over them and decide that overlay doesn't need to implement them? I doubt it, but if you did, please document that. > struct ovl_file_entry { > struct file *realfile; > - void *vm_ops; > + const struct vm_operations_struct *vm_ops; > }; > > struct file *ovl_get_realfile(struct file *file) > @@ -40,14 +48,15 @@ void ovl_set_realfile(struct file *file, struct file *realfile) > ofe->realfile = realfile; > } > > -void *ovl_get_real_vmops(struct file *file) > +const struct vm_operations_struct *ovl_get_real_vmops(struct file *file) > { > struct ovl_file_entry *ofe = file->private_data; > > return ofe->vm_ops; > } > > -void ovl_set_real_vmops(struct file *file, void *vm_ops) > +void ovl_set_real_vmops(struct file *file, > + const struct vm_operations_struct *vm_ops) > { > struct ovl_file_entry *ofe = file->private_data; > > @@ -493,11 +502,104 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) > return ret; > } > > +vm_fault_t ovl_fault(struct vm_fault *vmf) > +{ > + struct vm_area_struct *vma = vmf->vma; > + struct file *file = vma->vm_file; > + struct file *realfile; > + struct file *fpin, *tmp; > + struct inode *inode = file_inode(file); > + struct inode *realinode; > + const struct cred *old_cred; > + bool retry_allowed; > + vm_fault_t ret; > + int err = 0; > + > + if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { > + realfile = ovl_get_realfile(file); > + > + if (!ovl_has_upperdata(inode) || > + realfile->f_inode != ovl_inode_upper(inode) || > + !realfile->f_op->mmap) > + return VM_FAULT_SIGBUS; > + > + if (!ovl_get_real_vmops(file)) { > + old_cred = ovl_override_creds(inode->i_sb); > + err = call_mmap(realfile, vma); > + revert_creds(old_cred); > + > + vma->vm_file = file; > + if (err) { > + vma->vm_ops = &ovl_vm_ops; > + return VM_FAULT_SIGBUS; > + } > + ovl_set_real_vmops(file, vma->vm_ops); > + vma->vm_ops = &ovl_vm_ops; > + } > + > + retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); > + if (retry_allowed) > + vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; > + vma->vm_file = realfile; > + ret = ovl_get_real_vmops(file)->fault(vmf); > + vma->vm_file = file; > + if (retry_allowed) > + vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; > + return ret; > + > + } else { > + fpin = maybe_unlock_mmap_for_io(vmf, NULL); > + if (!fpin) > + return VM_FAULT_SIGBUS; > + > + ret = VM_FAULT_RETRY; > + if (!ovl_has_upperdata(inode)) { > + err = ovl_copy_up_with_data(file->f_path.dentry); > + if (err) > + goto out; > + } > + > + realinode = ovl_inode_realdata(inode); > + realfile = ovl_open_realfile(file, realinode); > + if (IS_ERR(realfile)) > + goto out; > + > + tmp = ovl_get_realfile(file); > + ovl_set_realfile(file, realfile); > + fput(tmp); > + > +out: > + fput(fpin); > + return ret; > + } > +} Please add some documentation to explain the method used. Do we need to retry if real_vmops are already set? Thanks, Amir. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map @ 2020-08-30 11:33 ` Amir Goldstein 0 siblings, 0 replies; 12+ messages in thread From: Amir Goldstein @ 2020-08-30 11:33 UTC (permalink / raw) To: Chengguang Xu Cc: overlayfs, Linux MM, Miklos Szeredi, Andrew Morton, Ritesh Harjani On Sat, Aug 29, 2020 at 12:51 PM Chengguang Xu <cgxu519@mykernel.net> wrote: > > Implement stacked mmap for shared map to keep data > consistency. > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> > --- > fs/overlayfs/file.c | 120 +++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 114 insertions(+), 6 deletions(-) > > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c > index 14ab5344a918..db5ab200d984 100644 > --- a/fs/overlayfs/file.c > +++ b/fs/overlayfs/file.c > @@ -21,9 +21,17 @@ struct ovl_aio_req { > struct fd fd; > }; > > +static vm_fault_t ovl_fault(struct vm_fault *vmf); > +static vm_fault_t ovl_page_mkwrite(struct vm_fault *vmf); > + > +static const struct vm_operations_struct ovl_vm_ops = { > + .fault = ovl_fault, > + .page_mkwrite = ovl_page_mkwrite, > +}; > + Interesting direction, not sure if this is workable. I don't know enough about mm to say. But what about the rest of the operations? Did you go over them and decide that overlay doesn't need to implement them? I doubt it, but if you did, please document that. > struct ovl_file_entry { > struct file *realfile; > - void *vm_ops; > + const struct vm_operations_struct *vm_ops; > }; > > struct file *ovl_get_realfile(struct file *file) > @@ -40,14 +48,15 @@ void ovl_set_realfile(struct file *file, struct file *realfile) > ofe->realfile = realfile; > } > > -void *ovl_get_real_vmops(struct file *file) > +const struct vm_operations_struct *ovl_get_real_vmops(struct file *file) > { > struct ovl_file_entry *ofe = file->private_data; > > return ofe->vm_ops; > } > > -void ovl_set_real_vmops(struct file *file, void *vm_ops) > +void ovl_set_real_vmops(struct file *file, > + const struct vm_operations_struct *vm_ops) > { > struct ovl_file_entry *ofe = file->private_data; > > @@ -493,11 +502,104 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) > return ret; > } > > +vm_fault_t ovl_fault(struct vm_fault *vmf) > +{ > + struct vm_area_struct *vma = vmf->vma; > + struct file *file = vma->vm_file; > + struct file *realfile; > + struct file *fpin, *tmp; > + struct inode *inode = file_inode(file); > + struct inode *realinode; > + const struct cred *old_cred; > + bool retry_allowed; > + vm_fault_t ret; > + int err = 0; > + > + if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { > + realfile = ovl_get_realfile(file); > + > + if (!ovl_has_upperdata(inode) || > + realfile->f_inode != ovl_inode_upper(inode) || > + !realfile->f_op->mmap) > + return VM_FAULT_SIGBUS; > + > + if (!ovl_get_real_vmops(file)) { > + old_cred = ovl_override_creds(inode->i_sb); > + err = call_mmap(realfile, vma); > + revert_creds(old_cred); > + > + vma->vm_file = file; > + if (err) { > + vma->vm_ops = &ovl_vm_ops; > + return VM_FAULT_SIGBUS; > + } > + ovl_set_real_vmops(file, vma->vm_ops); > + vma->vm_ops = &ovl_vm_ops; > + } > + > + retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); > + if (retry_allowed) > + vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; > + vma->vm_file = realfile; > + ret = ovl_get_real_vmops(file)->fault(vmf); > + vma->vm_file = file; > + if (retry_allowed) > + vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; > + return ret; > + > + } else { > + fpin = maybe_unlock_mmap_for_io(vmf, NULL); > + if (!fpin) > + return VM_FAULT_SIGBUS; > + > + ret = VM_FAULT_RETRY; > + if (!ovl_has_upperdata(inode)) { > + err = ovl_copy_up_with_data(file->f_path.dentry); > + if (err) > + goto out; > + } > + > + realinode = ovl_inode_realdata(inode); > + realfile = ovl_open_realfile(file, realinode); > + if (IS_ERR(realfile)) > + goto out; > + > + tmp = ovl_get_realfile(file); > + ovl_set_realfile(file, realfile); > + fput(tmp); > + > +out: > + fput(fpin); > + return ret; > + } > +} Please add some documentation to explain the method used. Do we need to retry if real_vmops are already set? Thanks, Amir. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map 2020-08-30 11:33 ` Amir Goldstein (?) @ 2020-08-31 13:47 ` cgxu 2020-08-31 15:51 ` Amir Goldstein -1 siblings, 1 reply; 12+ messages in thread From: cgxu @ 2020-08-31 13:47 UTC (permalink / raw) To: Amir Goldstein Cc: overlayfs, Linux MM, Miklos Szeredi, Andrew Morton, Ritesh Harjani On 8/30/20 7:33 PM, Amir Goldstein wrote: > On Sat, Aug 29, 2020 at 12:51 PM Chengguang Xu <cgxu519@mykernel.net> wrote: >> >> Implement stacked mmap for shared map to keep data >> consistency. >> >> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> >> --- >> fs/overlayfs/file.c | 120 +++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 114 insertions(+), 6 deletions(-) >> >> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c >> index 14ab5344a918..db5ab200d984 100644 >> --- a/fs/overlayfs/file.c >> +++ b/fs/overlayfs/file.c >> @@ -21,9 +21,17 @@ struct ovl_aio_req { >> struct fd fd; >> }; >> >> +static vm_fault_t ovl_fault(struct vm_fault *vmf); >> +static vm_fault_t ovl_page_mkwrite(struct vm_fault *vmf); >> + >> +static const struct vm_operations_struct ovl_vm_ops = { >> + .fault = ovl_fault, >> + .page_mkwrite = ovl_page_mkwrite, >> +}; >> + > > Interesting direction, not sure if this is workable. > I don't know enough about mm to say. > > But what about the rest of the operations? > Did you go over them and decide that overlay doesn't need to implement them? > I doubt it, but if you did, please document that. I did some check for rest of them, IIUC ->fault will be enough for this special case (shared read-only mmap with no upper), I will remove ->page_mkwrite in v2. # I do not consider support ->huge_fault in current stage due to many fs cannot support DAX properly. BTW, do you know who should I add to CC list for further deep review of this code? fadevel-list? > >> struct ovl_file_entry { >> struct file *realfile; >> - void *vm_ops; >> + const struct vm_operations_struct *vm_ops; >> }; >> >> struct file *ovl_get_realfile(struct file *file) >> @@ -40,14 +48,15 @@ void ovl_set_realfile(struct file *file, struct file *realfile) >> ofe->realfile = realfile; >> } >> >> -void *ovl_get_real_vmops(struct file *file) >> +const struct vm_operations_struct *ovl_get_real_vmops(struct file *file) >> { >> struct ovl_file_entry *ofe = file->private_data; >> >> return ofe->vm_ops; >> } >> >> -void ovl_set_real_vmops(struct file *file, void *vm_ops) >> +void ovl_set_real_vmops(struct file *file, >> + const struct vm_operations_struct *vm_ops) >> { >> struct ovl_file_entry *ofe = file->private_data; >> >> @@ -493,11 +502,104 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) >> return ret; >> } >> >> +vm_fault_t ovl_fault(struct vm_fault *vmf) >> +{ >> + struct vm_area_struct *vma = vmf->vma; >> + struct file *file = vma->vm_file; >> + struct file *realfile; >> + struct file *fpin, *tmp; >> + struct inode *inode = file_inode(file); >> + struct inode *realinode; >> + const struct cred *old_cred; >> + bool retry_allowed; >> + vm_fault_t ret; >> + int err = 0; >> + >> + if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { >> + realfile = ovl_get_realfile(file); >> + >> + if (!ovl_has_upperdata(inode) || >> + realfile->f_inode != ovl_inode_upper(inode) || >> + !realfile->f_op->mmap) >> + return VM_FAULT_SIGBUS; >> + >> + if (!ovl_get_real_vmops(file)) { >> + old_cred = ovl_override_creds(inode->i_sb); >> + err = call_mmap(realfile, vma); >> + revert_creds(old_cred); >> + >> + vma->vm_file = file; >> + if (err) { >> + vma->vm_ops = &ovl_vm_ops; >> + return VM_FAULT_SIGBUS; >> + } >> + ovl_set_real_vmops(file, vma->vm_ops); >> + vma->vm_ops = &ovl_vm_ops; >> + } >> + >> + retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); >> + if (retry_allowed) >> + vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; >> + vma->vm_file = realfile; >> + ret = ovl_get_real_vmops(file)->fault(vmf); >> + vma->vm_file = file; >> + if (retry_allowed) >> + vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; >> + return ret; >> + >> + } else { >> + fpin = maybe_unlock_mmap_for_io(vmf, NULL); >> + if (!fpin) >> + return VM_FAULT_SIGBUS; >> + >> + ret = VM_FAULT_RETRY; >> + if (!ovl_has_upperdata(inode)) { >> + err = ovl_copy_up_with_data(file->f_path.dentry); >> + if (err) >> + goto out; >> + } >> + >> + realinode = ovl_inode_realdata(inode); >> + realfile = ovl_open_realfile(file, realinode); >> + if (IS_ERR(realfile)) >> + goto out; >> + >> + tmp = ovl_get_realfile(file); >> + ovl_set_realfile(file, realfile); >> + fput(tmp); >> + >> +out: >> + fput(fpin); >> + return ret; >> + } >> +} > > > Please add some documentation to explain the method used. > Do we need to retry if real_vmops are already set? > Good catch, actually retry is not needed in that case. Basically, we unlock(mmap_lock)->copy-up->open when detecting no upper inode then retry fault operation. However, we need to check fault retry flag carefully for avoiding endless retry. I'll add more explanation in v2. --- cgxu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map 2020-08-31 13:47 ` cgxu @ 2020-08-31 15:51 ` Amir Goldstein 0 siblings, 0 replies; 12+ messages in thread From: Amir Goldstein @ 2020-08-31 15:51 UTC (permalink / raw) To: cgxu; +Cc: overlayfs, Linux MM, Miklos Szeredi, Andrew Morton, Ritesh Harjani On Mon, Aug 31, 2020 at 4:47 PM cgxu <cgxu519@mykernel.net> wrote: > > On 8/30/20 7:33 PM, Amir Goldstein wrote: > > On Sat, Aug 29, 2020 at 12:51 PM Chengguang Xu <cgxu519@mykernel.net> wrote: > >> > >> Implement stacked mmap for shared map to keep data > >> consistency. > >> > >> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> > >> --- > >> fs/overlayfs/file.c | 120 +++++++++++++++++++++++++++++++++++++++++--- > >> 1 file changed, 114 insertions(+), 6 deletions(-) > >> > >> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c > >> index 14ab5344a918..db5ab200d984 100644 > >> --- a/fs/overlayfs/file.c > >> +++ b/fs/overlayfs/file.c > >> @@ -21,9 +21,17 @@ struct ovl_aio_req { > >> struct fd fd; > >> }; > >> > >> +static vm_fault_t ovl_fault(struct vm_fault *vmf); > >> +static vm_fault_t ovl_page_mkwrite(struct vm_fault *vmf); > >> + > >> +static const struct vm_operations_struct ovl_vm_ops = { > >> + .fault = ovl_fault, > >> + .page_mkwrite = ovl_page_mkwrite, > >> +}; > >> + > > > > Interesting direction, not sure if this is workable. > > I don't know enough about mm to say. > > > > But what about the rest of the operations? > > Did you go over them and decide that overlay doesn't need to implement them? > > I doubt it, but if you did, please document that. > > I did some check for rest of them, IIUC ->fault will be enough for this > special case (shared read-only mmap with no upper), I will remove > ->page_mkwrite in v2. Ok I suppose you checked that ->map_pages is not relevant? > > # I do not consider support ->huge_fault in current stage due to many fs > cannot support DAX properly. > > BTW, do you know who should I add to CC list for further deep review of > this code? fadevel-list? > fsdevel would be good, but I would wait for initial feedback from Miklos before you post v2... > > > > > >> struct ovl_file_entry { > >> struct file *realfile; > >> - void *vm_ops; > >> + const struct vm_operations_struct *vm_ops; > >> }; > >> > >> struct file *ovl_get_realfile(struct file *file) > >> @@ -40,14 +48,15 @@ void ovl_set_realfile(struct file *file, struct file *realfile) > >> ofe->realfile = realfile; > >> } > >> > >> -void *ovl_get_real_vmops(struct file *file) > >> +const struct vm_operations_struct *ovl_get_real_vmops(struct file *file) > >> { > >> struct ovl_file_entry *ofe = file->private_data; > >> > >> return ofe->vm_ops; > >> } > >> > >> -void ovl_set_real_vmops(struct file *file, void *vm_ops) > >> +void ovl_set_real_vmops(struct file *file, > >> + const struct vm_operations_struct *vm_ops) > >> { > >> struct ovl_file_entry *ofe = file->private_data; > >> > >> @@ -493,11 +502,104 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) > >> return ret; > >> } > >> > >> +vm_fault_t ovl_fault(struct vm_fault *vmf) > >> +{ > >> + struct vm_area_struct *vma = vmf->vma; > >> + struct file *file = vma->vm_file; > >> + struct file *realfile; > >> + struct file *fpin, *tmp; > >> + struct inode *inode = file_inode(file); > >> + struct inode *realinode; > >> + const struct cred *old_cred; > >> + bool retry_allowed; > >> + vm_fault_t ret; > >> + int err = 0; > >> + > >> + if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { > >> + realfile = ovl_get_realfile(file); > >> + > >> + if (!ovl_has_upperdata(inode) || > >> + realfile->f_inode != ovl_inode_upper(inode) || > >> + !realfile->f_op->mmap) > >> + return VM_FAULT_SIGBUS; > >> + > >> + if (!ovl_get_real_vmops(file)) { > >> + old_cred = ovl_override_creds(inode->i_sb); > >> + err = call_mmap(realfile, vma); > >> + revert_creds(old_cred); > >> + > >> + vma->vm_file = file; > >> + if (err) { > >> + vma->vm_ops = &ovl_vm_ops; > >> + return VM_FAULT_SIGBUS; > >> + } > >> + ovl_set_real_vmops(file, vma->vm_ops); > >> + vma->vm_ops = &ovl_vm_ops; > >> + } > >> + > >> + retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); > >> + if (retry_allowed) > >> + vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; > >> + vma->vm_file = realfile; > >> + ret = ovl_get_real_vmops(file)->fault(vmf); > >> + vma->vm_file = file; > >> + if (retry_allowed) > >> + vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; > >> + return ret; > >> + > >> + } else { > >> + fpin = maybe_unlock_mmap_for_io(vmf, NULL); > >> + if (!fpin) > >> + return VM_FAULT_SIGBUS; > >> + > >> + ret = VM_FAULT_RETRY; > >> + if (!ovl_has_upperdata(inode)) { > >> + err = ovl_copy_up_with_data(file->f_path.dentry); > >> + if (err) > >> + goto out; > >> + } > >> + > >> + realinode = ovl_inode_realdata(inode); > >> + realfile = ovl_open_realfile(file, realinode); > >> + if (IS_ERR(realfile)) > >> + goto out; > >> + > >> + tmp = ovl_get_realfile(file); > >> + ovl_set_realfile(file, realfile); > >> + fput(tmp); > >> + > >> +out: > >> + fput(fpin); > >> + return ret; > >> + } > >> +} > > > > > > Please add some documentation to explain the method used. > > Do we need to retry if real_vmops are already set? > > > > Good catch, actually retry is not needed in that case. > > Basically, we unlock(mmap_lock)->copy-up->open when > detecting no upper inode then retry fault operation. > However, we need to check fault retry flag carefully > for avoiding endless retry. That much I got, but the details of setting ->vm_file and vmops look subtle, so better explain them. Thanks, Amir. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map @ 2020-08-31 15:51 ` Amir Goldstein 0 siblings, 0 replies; 12+ messages in thread From: Amir Goldstein @ 2020-08-31 15:51 UTC (permalink / raw) To: cgxu; +Cc: overlayfs, Linux MM, Miklos Szeredi, Andrew Morton, Ritesh Harjani On Mon, Aug 31, 2020 at 4:47 PM cgxu <cgxu519@mykernel.net> wrote: > > On 8/30/20 7:33 PM, Amir Goldstein wrote: > > On Sat, Aug 29, 2020 at 12:51 PM Chengguang Xu <cgxu519@mykernel.net> wrote: > >> > >> Implement stacked mmap for shared map to keep data > >> consistency. > >> > >> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> > >> --- > >> fs/overlayfs/file.c | 120 +++++++++++++++++++++++++++++++++++++++++--- > >> 1 file changed, 114 insertions(+), 6 deletions(-) > >> > >> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c > >> index 14ab5344a918..db5ab200d984 100644 > >> --- a/fs/overlayfs/file.c > >> +++ b/fs/overlayfs/file.c > >> @@ -21,9 +21,17 @@ struct ovl_aio_req { > >> struct fd fd; > >> }; > >> > >> +static vm_fault_t ovl_fault(struct vm_fault *vmf); > >> +static vm_fault_t ovl_page_mkwrite(struct vm_fault *vmf); > >> + > >> +static const struct vm_operations_struct ovl_vm_ops = { > >> + .fault = ovl_fault, > >> + .page_mkwrite = ovl_page_mkwrite, > >> +}; > >> + > > > > Interesting direction, not sure if this is workable. > > I don't know enough about mm to say. > > > > But what about the rest of the operations? > > Did you go over them and decide that overlay doesn't need to implement them? > > I doubt it, but if you did, please document that. > > I did some check for rest of them, IIUC ->fault will be enough for this > special case (shared read-only mmap with no upper), I will remove > ->page_mkwrite in v2. Ok I suppose you checked that ->map_pages is not relevant? > > # I do not consider support ->huge_fault in current stage due to many fs > cannot support DAX properly. > > BTW, do you know who should I add to CC list for further deep review of > this code? fadevel-list? > fsdevel would be good, but I would wait for initial feedback from Miklos before you post v2... > > > > > >> struct ovl_file_entry { > >> struct file *realfile; > >> - void *vm_ops; > >> + const struct vm_operations_struct *vm_ops; > >> }; > >> > >> struct file *ovl_get_realfile(struct file *file) > >> @@ -40,14 +48,15 @@ void ovl_set_realfile(struct file *file, struct file *realfile) > >> ofe->realfile = realfile; > >> } > >> > >> -void *ovl_get_real_vmops(struct file *file) > >> +const struct vm_operations_struct *ovl_get_real_vmops(struct file *file) > >> { > >> struct ovl_file_entry *ofe = file->private_data; > >> > >> return ofe->vm_ops; > >> } > >> > >> -void ovl_set_real_vmops(struct file *file, void *vm_ops) > >> +void ovl_set_real_vmops(struct file *file, > >> + const struct vm_operations_struct *vm_ops) > >> { > >> struct ovl_file_entry *ofe = file->private_data; > >> > >> @@ -493,11 +502,104 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) > >> return ret; > >> } > >> > >> +vm_fault_t ovl_fault(struct vm_fault *vmf) > >> +{ > >> + struct vm_area_struct *vma = vmf->vma; > >> + struct file *file = vma->vm_file; > >> + struct file *realfile; > >> + struct file *fpin, *tmp; > >> + struct inode *inode = file_inode(file); > >> + struct inode *realinode; > >> + const struct cred *old_cred; > >> + bool retry_allowed; > >> + vm_fault_t ret; > >> + int err = 0; > >> + > >> + if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { > >> + realfile = ovl_get_realfile(file); > >> + > >> + if (!ovl_has_upperdata(inode) || > >> + realfile->f_inode != ovl_inode_upper(inode) || > >> + !realfile->f_op->mmap) > >> + return VM_FAULT_SIGBUS; > >> + > >> + if (!ovl_get_real_vmops(file)) { > >> + old_cred = ovl_override_creds(inode->i_sb); > >> + err = call_mmap(realfile, vma); > >> + revert_creds(old_cred); > >> + > >> + vma->vm_file = file; > >> + if (err) { > >> + vma->vm_ops = &ovl_vm_ops; > >> + return VM_FAULT_SIGBUS; > >> + } > >> + ovl_set_real_vmops(file, vma->vm_ops); > >> + vma->vm_ops = &ovl_vm_ops; > >> + } > >> + > >> + retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); > >> + if (retry_allowed) > >> + vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; > >> + vma->vm_file = realfile; > >> + ret = ovl_get_real_vmops(file)->fault(vmf); > >> + vma->vm_file = file; > >> + if (retry_allowed) > >> + vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; > >> + return ret; > >> + > >> + } else { > >> + fpin = maybe_unlock_mmap_for_io(vmf, NULL); > >> + if (!fpin) > >> + return VM_FAULT_SIGBUS; > >> + > >> + ret = VM_FAULT_RETRY; > >> + if (!ovl_has_upperdata(inode)) { > >> + err = ovl_copy_up_with_data(file->f_path.dentry); > >> + if (err) > >> + goto out; > >> + } > >> + > >> + realinode = ovl_inode_realdata(inode); > >> + realfile = ovl_open_realfile(file, realinode); > >> + if (IS_ERR(realfile)) > >> + goto out; > >> + > >> + tmp = ovl_get_realfile(file); > >> + ovl_set_realfile(file, realfile); > >> + fput(tmp); > >> + > >> +out: > >> + fput(fpin); > >> + return ret; > >> + } > >> +} > > > > > > Please add some documentation to explain the method used. > > Do we need to retry if real_vmops are already set? > > > > Good catch, actually retry is not needed in that case. > > Basically, we unlock(mmap_lock)->copy-up->open when > detecting no upper inode then retry fault operation. > However, we need to check fault retry flag carefully > for avoiding endless retry. That much I got, but the details of setting ->vm_file and vmops look subtle, so better explain them. Thanks, Amir. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map 2020-08-31 15:51 ` Amir Goldstein @ 2020-09-01 7:44 ` Miklos Szeredi -1 siblings, 0 replies; 12+ messages in thread From: Miklos Szeredi @ 2020-09-01 7:44 UTC (permalink / raw) To: Amir Goldstein; +Cc: cgxu, overlayfs, Linux MM, Andrew Morton, Ritesh Harjani On Mon, Aug 31, 2020 at 5:52 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Mon, Aug 31, 2020 at 4:47 PM cgxu <cgxu519@mykernel.net> wrote: > > > > On 8/30/20 7:33 PM, Amir Goldstein wrote: > > > Interesting direction, not sure if this is workable. > > > I don't know enough about mm to say. > > > > > > But what about the rest of the operations? > > > Did you go over them and decide that overlay doesn't need to implement them? > > > I doubt it, but if you did, please document that. > > > > I did some check for rest of them, IIUC ->fault will be enough for this > > special case (shared read-only mmap with no upper), I will remove > > ->page_mkwrite in v2. Hmm, so this is just for that specific corner case? Not a generic shared mmap implementation? > > Ok I suppose you checked that ->map_pages is not relevant? > > > > > # I do not consider support ->huge_fault in current stage due to many fs > > cannot support DAX properly. > > > > BTW, do you know who should I add to CC list for further deep review of > > this code? fadevel-list? > > > > fsdevel would be good, but I would wait for initial feedback from Miklos > before you post v2... I could dig into the details of page fault handling, but that's not an area that I'm intimately familiar with. So yeah, a high level description of what happens on mmap, page fault, write fault, etc... would be really appreciated. Thanks, Miklos ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map @ 2020-09-01 7:44 ` Miklos Szeredi 0 siblings, 0 replies; 12+ messages in thread From: Miklos Szeredi @ 2020-09-01 7:44 UTC (permalink / raw) To: Amir Goldstein; +Cc: cgxu, overlayfs, Linux MM, Andrew Morton, Ritesh Harjani On Mon, Aug 31, 2020 at 5:52 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Mon, Aug 31, 2020 at 4:47 PM cgxu <cgxu519@mykernel.net> wrote: > > > > On 8/30/20 7:33 PM, Amir Goldstein wrote: > > > Interesting direction, not sure if this is workable. > > > I don't know enough about mm to say. > > > > > > But what about the rest of the operations? > > > Did you go over them and decide that overlay doesn't need to implement them? > > > I doubt it, but if you did, please document that. > > > > I did some check for rest of them, IIUC ->fault will be enough for this > > special case (shared read-only mmap with no upper), I will remove > > ->page_mkwrite in v2. Hmm, so this is just for that specific corner case? Not a generic shared mmap implementation? > > Ok I suppose you checked that ->map_pages is not relevant? > > > > > # I do not consider support ->huge_fault in current stage due to many fs > > cannot support DAX properly. > > > > BTW, do you know who should I add to CC list for further deep review of > > this code? fadevel-list? > > > > fsdevel would be good, but I would wait for initial feedback from Miklos > before you post v2... I could dig into the details of page fault handling, but that's not an area that I'm intimately familiar with. So yeah, a high level description of what happens on mmap, page fault, write fault, etc... would be really appreciated. Thanks, Miklos ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map 2020-08-31 15:51 ` Amir Goldstein (?) (?) @ 2020-09-01 13:20 ` cgxu -1 siblings, 0 replies; 12+ messages in thread From: cgxu @ 2020-09-01 13:20 UTC (permalink / raw) To: Amir Goldstein Cc: overlayfs, Linux MM, Miklos Szeredi, Andrew Morton, Ritesh Harjani On 8/31/20 11:51 PM, Amir Goldstein wrote: > On Mon, Aug 31, 2020 at 4:47 PM cgxu <cgxu519@mykernel.net> wrote: >> >> On 8/30/20 7:33 PM, Amir Goldstein wrote: >>> On Sat, Aug 29, 2020 at 12:51 PM Chengguang Xu <cgxu519@mykernel.net> wrote: >>>> >>>> Implement stacked mmap for shared map to keep data >>>> consistency. >>>> >>>> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> >>>> --- >>>> fs/overlayfs/file.c | 120 +++++++++++++++++++++++++++++++++++++++++--- >>>> 1 file changed, 114 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c >>>> index 14ab5344a918..db5ab200d984 100644 >>>> --- a/fs/overlayfs/file.c >>>> +++ b/fs/overlayfs/file.c >>>> @@ -21,9 +21,17 @@ struct ovl_aio_req { >>>> struct fd fd; >>>> }; >>>> >>>> +static vm_fault_t ovl_fault(struct vm_fault *vmf); >>>> +static vm_fault_t ovl_page_mkwrite(struct vm_fault *vmf); >>>> + >>>> +static const struct vm_operations_struct ovl_vm_ops = { >>>> + .fault = ovl_fault, >>>> + .page_mkwrite = ovl_page_mkwrite, >>>> +}; >>>> + >>> >>> Interesting direction, not sure if this is workable. >>> I don't know enough about mm to say. >>> >>> But what about the rest of the operations? >>> Did you go over them and decide that overlay doesn't need to implement them? >>> I doubt it, but if you did, please document that. >> >> I did some check for rest of them, IIUC ->fault will be enough for this >> special case (shared read-only mmap with no upper), I will remove >> ->page_mkwrite in v2. > > Ok I suppose you checked that ->map_pages is not relevant? ->map_pages() does easy maps and fallback to ->fault if the offset is not ready, so I think without ->map_pages() it still could work properly, we can also implement it for acceleration. > >> >> # I do not consider support ->huge_fault in current stage due to many fs >> cannot support DAX properly. >> >> BTW, do you know who should I add to CC list for further deep review of >> this code? fadevel-list? >> > > fsdevel would be good, but I would wait for initial feedback from Miklos > before you post v2... > >> >> >>> >>>> struct ovl_file_entry { >>>> struct file *realfile; >>>> - void *vm_ops; >>>> + const struct vm_operations_struct *vm_ops; >>>> }; >>>> >>>> struct file *ovl_get_realfile(struct file *file) >>>> @@ -40,14 +48,15 @@ void ovl_set_realfile(struct file *file, struct file *realfile) >>>> ofe->realfile = realfile; >>>> } >>>> >>>> -void *ovl_get_real_vmops(struct file *file) >>>> +const struct vm_operations_struct *ovl_get_real_vmops(struct file *file) >>>> { >>>> struct ovl_file_entry *ofe = file->private_data; >>>> >>>> return ofe->vm_ops; >>>> } >>>> >>>> -void ovl_set_real_vmops(struct file *file, void *vm_ops) >>>> +void ovl_set_real_vmops(struct file *file, >>>> + const struct vm_operations_struct *vm_ops) >>>> { >>>> struct ovl_file_entry *ofe = file->private_data; >>>> >>>> @@ -493,11 +502,104 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) >>>> return ret; >>>> } >>>> >>>> +vm_fault_t ovl_fault(struct vm_fault *vmf) >>>> +{ >>>> + struct vm_area_struct *vma = vmf->vma; >>>> + struct file *file = vma->vm_file; >>>> + struct file *realfile; >>>> + struct file *fpin, *tmp; >>>> + struct inode *inode = file_inode(file); >>>> + struct inode *realinode; >>>> + const struct cred *old_cred; >>>> + bool retry_allowed; >>>> + vm_fault_t ret; >>>> + int err = 0; >>>> + >>>> + if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { >>>> + realfile = ovl_get_realfile(file); >>>> + >>>> + if (!ovl_has_upperdata(inode) || >>>> + realfile->f_inode != ovl_inode_upper(inode) || >>>> + !realfile->f_op->mmap) >>>> + return VM_FAULT_SIGBUS; >>>> + >>>> + if (!ovl_get_real_vmops(file)) { >>>> + old_cred = ovl_override_creds(inode->i_sb); >>>> + err = call_mmap(realfile, vma); >>>> + revert_creds(old_cred); >>>> + >>>> + vma->vm_file = file; >>>> + if (err) { >>>> + vma->vm_ops = &ovl_vm_ops; >>>> + return VM_FAULT_SIGBUS; >>>> + } >>>> + ovl_set_real_vmops(file, vma->vm_ops); >>>> + vma->vm_ops = &ovl_vm_ops; >>>> + } >>>> + >>>> + retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); >>>> + if (retry_allowed) >>>> + vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; >>>> + vma->vm_file = realfile; >>>> + ret = ovl_get_real_vmops(file)->fault(vmf); >>>> + vma->vm_file = file; >>>> + if (retry_allowed) >>>> + vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; >>>> + return ret; >>>> + >>>> + } else { >>>> + fpin = maybe_unlock_mmap_for_io(vmf, NULL); >>>> + if (!fpin) >>>> + return VM_FAULT_SIGBUS; >>>> + >>>> + ret = VM_FAULT_RETRY; >>>> + if (!ovl_has_upperdata(inode)) { >>>> + err = ovl_copy_up_with_data(file->f_path.dentry); >>>> + if (err) >>>> + goto out; >>>> + } >>>> + >>>> + realinode = ovl_inode_realdata(inode); >>>> + realfile = ovl_open_realfile(file, realinode); >>>> + if (IS_ERR(realfile)) >>>> + goto out; >>>> + >>>> + tmp = ovl_get_realfile(file); >>>> + ovl_set_realfile(file, realfile); >>>> + fput(tmp); >>>> + >>>> +out: >>>> + fput(fpin); >>>> + return ret; >>>> + } >>>> +} >>> >>> >>> Please add some documentation to explain the method used. >>> Do we need to retry if real_vmops are already set? >>> >> >> Good catch, actually retry is not needed in that case. >> >> Basically, we unlock(mmap_lock)->copy-up->open when >> detecting no upper inode then retry fault operation. >> However, we need to check fault retry flag carefully >> for avoiding endless retry. > > That much I got, but the details of setting ->vm_file and vmops > look subtle, so better explain them. > I'll add some explanations in V2, but before that let me write some comments based on code logic below. If there is still something not clear you can point out that again. + if (fault_flag_check(vmf, FAULT_FLAG_TRIED)) { + realfile = ovl_get_realfile(file); + + if (!ovl_has_upperdata(inode) || + realfile->f_inode != ovl_inode_upper(inode) || + !realfile->f_op->mmap) + return VM_FAULT_SIGBUS; Above condition indicates (copy-up)/(open real-file) failed or (real-file does not support mmap), so we have to return SIGBUS. + + if (!ovl_get_real_vmops(file)) { + old_cred = ovl_override_creds(inode->i_sb); + err = call_mmap(realfile, vma); + revert_creds(old_cred); + + vma->vm_file = file; + if (err) { + vma->vm_ops = &ovl_vm_ops; + return VM_FAULT_SIGBUS; + } + ovl_set_real_vmops(file, vma->vm_ops); + vma->vm_ops = &ovl_vm_ops; call_mmap() will rewrite vma->vm_file and vma->vm_ops to upper layer's, so here recover to overlay's in order to jump into this ovl_fault() in other page-faults. + } + + retry_allowed = fault_flag_check(vmf, FAULT_FLAG_ALLOW_RETRY); + if (retry_allowed) + vma->vm_flags &= ~FAULT_FLAG_ALLOW_RETRY; here, we disallow retry in real ->fault because retry will unlock mmap_lock, touching vma after unlock is not safe behavior. + vma->vm_file = realfile; + ret = ovl_get_real_vmops(file)->fault(vmf); calling real fault handler. + vma->vm_file = file; + if (retry_allowed) + vma->vm_flags |= FAULT_FLAG_ALLOW_RETRY; recover vm_file and vm_ops to overlay's so that we can jump into ovl_fault in other page-faults. + return ret; + + } else { Thanks, cgxu ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-10 0:39 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <4ac1b013-6193-da70-3384-d8878a247fbc@mykernel.net> 2020-08-30 7:10 ` [RFC PATCH 3/3] ovl: implement stacked mmap for shared map Chen, Rong A 2020-09-10 0:39 ` Rong Chen 2020-08-29 9:50 [RFC PATCH 0/3] ovl: " Chengguang Xu 2020-08-29 9:51 ` [RFC PATCH 3/3] ovl: implement " Chengguang Xu 2020-08-29 11:35 ` kernel test robot 2020-08-30 11:33 ` Amir Goldstein 2020-08-30 11:33 ` Amir Goldstein 2020-08-31 13:47 ` cgxu 2020-08-31 15:51 ` Amir Goldstein 2020-08-31 15:51 ` Amir Goldstein 2020-09-01 7:44 ` Miklos Szeredi 2020-09-01 7:44 ` Miklos Szeredi 2020-09-01 13:20 ` cgxu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.