[PATCH] ext2: implement ->page_mkwrite

[PATCH] ext2: implement ->page_mkwrite

[Chengguang Xu]

Currently ext2 uses generic mmap operations for non DAX file and
filemap_page_mkwrite() does not check the block allocation for
shared writable mmapped area on pagefault. In some cases like
disk space exhaustion or disk quota limitation, it will cause silent
data loss. This patch tries to check and do block preallocation on
pagefault if necessary and explicitly return error to user when
allocation failure.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ext2/file.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index 96044f5dbc0e..a34119415ef1 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -25,10 +25,34 @@
 #include <linux/quotaops.h>
 #include <linux/iomap.h>
 #include <linux/uio.h>
+#include <linux/buffer_head.h>
 #include "ext2.h"
 #include "xattr.h"
 #include "acl.h"
 
+vm_fault_t ext2_page_mkwrite(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct inode *inode = file_inode(vma->vm_file);
+	int err;
+
+	if (unlikely(IS_IMMUTABLE(inode)))
+		return VM_FAULT_SIGBUS;
+
+	sb_start_pagefault(inode->i_sb);
+	file_update_time(vma->vm_file);
+	err = block_page_mkwrite(vma, vmf, ext2_get_block);
+	sb_end_pagefault(inode->i_sb);
+
+	return block_page_mkwrite_return(err);
+}
+
+const struct vm_operations_struct ext2_vm_ops = {
+	.fault		= filemap_fault,
+	.map_pages	= filemap_map_pages,
+	.page_mkwrite	= ext2_page_mkwrite,
+};
+
 #ifdef CONFIG_FS_DAX
 static ssize_t ext2_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
@@ -123,15 +147,23 @@ static const struct vm_operations_struct ext2_dax_vm_ops = {
 
 static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
+	file_accessed(file);
 	if (!IS_DAX(file_inode(file)))
-		return generic_file_mmap(file, vma);
+		vma->vm_ops = &ext2_vm_ops;
+	else
+		vma->vm_ops = &ext2_dax_vm_ops;
 
-	file_accessed(file);
-	vma->vm_ops = &ext2_dax_vm_ops;
 	return 0;
 }
+
 #else
-#define ext2_file_mmap	generic_file_mmap
+static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	file_accessed(file);
+	vma->vm_ops = &ext2_vm_ops;
+	return 0;
+}
+
 #endif
 
 /*
-- 
2.18.4

Re: [PATCH] ext2: implement ->page_mkwrite

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Hi Chengguang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.10 next-20201218]
[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/ext2-implement-page_mkwrite/20201218-214646
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a409ed156a90093a03fe6a93721ddf4c591eac87
config: x86_64-randconfig-p001-20201217 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/24259119fbeffce3b221c39c3b8bc0f2d5b1e750
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Chengguang-Xu/ext2-implement-page_mkwrite/20201218-214646
        git checkout 24259119fbeffce3b221c39c3b8bc0f2d5b1e750
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/ext2/file.c:33:12: warning: no previous prototype for 'ext2_page_mkwrite' [-Wmissing-prototypes]
      33 | vm_fault_t ext2_page_mkwrite(struct vm_fault *vmf)
         |            ^~~~~~~~~~~~~~~~~


vim +/ext2_page_mkwrite +33 fs/ext2/file.c

    32	
  > 33	vm_fault_t ext2_page_mkwrite(struct vm_fault *vmf)
    34	{
    35		struct vm_area_struct *vma = vmf->vma;
    36		struct inode *inode = file_inode(vma->vm_file);
    37		int err;
    38	
    39		if (unlikely(IS_IMMUTABLE(inode)))
    40			return VM_FAULT_SIGBUS;
    41	
    42		sb_start_pagefault(inode->i_sb);
    43		file_update_time(vma->vm_file);
    44		err = block_page_mkwrite(vma, vmf, ext2_get_block);
    45		sb_end_pagefault(inode->i_sb);
    46	
    47		return block_page_mkwrite_return(err);
    48	}
    49	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34886 bytes --]

Re: [PATCH] ext2: implement ->page_mkwrite

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]

Hi Chengguang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.10 next-20201223]
[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/ext2-implement-page_mkwrite/20201218-214646
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a409ed156a90093a03fe6a93721ddf4c591eac87
config: i386-randconfig-s002-20201217 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-184-g1b896707-dirty
        # https://github.com/0day-ci/linux/commit/24259119fbeffce3b221c39c3b8bc0f2d5b1e750
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Chengguang-Xu/ext2-implement-page_mkwrite/20201218-214646
        git checkout 24259119fbeffce3b221c39c3b8bc0f2d5b1e750
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> fs/ext2/file.c:33:12: sparse: sparse: symbol 'ext2_page_mkwrite' was not declared. Should it be static?
>> fs/ext2/file.c:50:35: sparse: sparse: symbol 'ext2_vm_ops' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37959 bytes --]

[RFC PATCH] ext2: ext2_page_mkwrite() can be static

[kernel test robot]

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 file.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index a34119415ef16d..4a5dbf349f6e13 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -30,7 +30,7 @@
 #include "xattr.h"
 #include "acl.h"
 
-vm_fault_t ext2_page_mkwrite(struct vm_fault *vmf)
+static vm_fault_t ext2_page_mkwrite(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct inode *inode = file_inode(vma->vm_file);
@@ -47,7 +47,7 @@ vm_fault_t ext2_page_mkwrite(struct vm_fault *vmf)
 	return block_page_mkwrite_return(err);
 }
 
-const struct vm_operations_struct ext2_vm_ops = {
+static const struct vm_operations_struct ext2_vm_ops = {
 	.fault		= filemap_fault,
 	.map_pages	= filemap_map_pages,
 	.page_mkwrite	= ext2_page_mkwrite,

Re: [PATCH] ext2: implement ->page_mkwrite

[Jan Kara]

On Fri 18-12-20 21:27:57, Chengguang Xu wrote:
> Currently ext2 uses generic mmap operations for non DAX file and
> filemap_page_mkwrite() does not check the block allocation for
> shared writable mmapped area on pagefault. In some cases like
> disk space exhaustion or disk quota limitation, it will cause silent
> data loss. This patch tries to check and do block preallocation on
> pagefault if necessary and explicitly return error to user when
> allocation failure.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>

Thanks for the patch and sorry for the delay in replying. I agree there's
the problem you describe but I'm not sure whether we should fix it.  ext2
has been like this since the beginning so well over 20 years.  Allocating
blocks on write fault has the unwelcome impact that the file layout is
likely going to be much worse (much more fragmented) - I remember getting
some reports about performance regressions from users back when I did a
similar change for ext3. And so I'm reluctant to change behavior of such
an old legacy filesystem as ext2...

								Honza

> ---
>  fs/ext2/file.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> index 96044f5dbc0e..a34119415ef1 100644
> --- a/fs/ext2/file.c
> +++ b/fs/ext2/file.c
> @@ -25,10 +25,34 @@
>  #include <linux/quotaops.h>
>  #include <linux/iomap.h>
>  #include <linux/uio.h>
> +#include <linux/buffer_head.h>
>  #include "ext2.h"
>  #include "xattr.h"
>  #include "acl.h"
>  
> +vm_fault_t ext2_page_mkwrite(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct inode *inode = file_inode(vma->vm_file);
> +	int err;
> +
> +	if (unlikely(IS_IMMUTABLE(inode)))
> +		return VM_FAULT_SIGBUS;
> +
> +	sb_start_pagefault(inode->i_sb);
> +	file_update_time(vma->vm_file);
> +	err = block_page_mkwrite(vma, vmf, ext2_get_block);
> +	sb_end_pagefault(inode->i_sb);
> +
> +	return block_page_mkwrite_return(err);
> +}
> +
> +const struct vm_operations_struct ext2_vm_ops = {
> +	.fault		= filemap_fault,
> +	.map_pages	= filemap_map_pages,
> +	.page_mkwrite	= ext2_page_mkwrite,
> +};
> +
>  #ifdef CONFIG_FS_DAX
>  static ssize_t ext2_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  {
> @@ -123,15 +147,23 @@ static const struct vm_operations_struct ext2_dax_vm_ops = {
>  
>  static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
>  {
> +	file_accessed(file);
>  	if (!IS_DAX(file_inode(file)))
> -		return generic_file_mmap(file, vma);
> +		vma->vm_ops = &ext2_vm_ops;
> +	else
> +		vma->vm_ops = &ext2_dax_vm_ops;
>  
> -	file_accessed(file);
> -	vma->vm_ops = &ext2_dax_vm_ops;
>  	return 0;
>  }
> +
>  #else
> -#define ext2_file_mmap	generic_file_mmap
> +static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	file_accessed(file);
> +	vma->vm_ops = &ext2_vm_ops;
> +	return 0;
> +}
> +
>  #endif
>  
>  /*
> -- 
> 2.18.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR