Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[Chen, Rong A]

[-- 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
>>
>

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[Rong Chen]

[-- 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
>>
>

[RFC PATCH 0/3] ovl: stacked mmap for shared map

[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

[RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[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

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[kernel test robot]

[-- 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 --]

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[Amir Goldstein]

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.

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[Amir Goldstein]

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.

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[cgxu]

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

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[Amir Goldstein]

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.

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[Amir Goldstein]

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.

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[Miklos Szeredi]

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

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[Miklos Szeredi]

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

Re: [RFC PATCH 3/3] ovl: implement stacked mmap for shared map

[cgxu]

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