Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Salvatore Bonaccorso <carnil@debian.org>]

Hi,

On Fri, Jan 12, 2024 at 11:20:53PM -0600, Steve French wrote:
> Here is a patch similar to what David suggested.  Seems
> straightforward fix.  See attached.
> I did limited testing on it tonight with 6.1 (will do more tomorrow,
> but feedback welcome) but it did fix the regression in xfstest
> generic/001 mentioned in this thread.
> 
> 
> 
> 
> On Fri, Jan 12, 2024 at 8:26 AM David Howells <dhowells@redhat.com> wrote:
> >
> > gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> wrote:
> >
> > > I guess I can just revert the single commit here?  Can someone send me
> > > the revert that I need to do so as I get it right?
> >
> > In cifs_flush_folio() the error check for filemap_get_folio() just needs
> > changing to check !folio instead of IS_ERR(folio).
> >
> > David
> >
> >
> 
> 
> --
> Thanks,
> 
> Steve

> From ba288a873fb8ac3d1bf5563366558a905620c071 Mon Sep 17 00:00:00 2001
> From: Steve French <stfrench@microsoft.com>
> Date: Fri, 12 Jan 2024 23:08:51 -0600
> Subject: [PATCH] cifs: fix flushing folio regression for 6.1 backport
> 
> filemap_get_folio works differenty in 6.1 vs. later kernels
> (returning NULL in 6.1 instead of an error).  Add
> this minor correction which addresses the regression in the patch:
>   cifs: Fix flushing, invalidation and file size with copy_file_range()
> 
> Suggested-by: David Howells <dhowells@redhat.com>
> Reported-by: Salvatore Bonaccorso <carnil@debian.org>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/smb/client/cifsfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 2e15b182e59f..ac0b7f229a23 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1240,7 +1240,7 @@ static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, lo
>  	int rc = 0;
>  
>  	folio = filemap_get_folio(inode->i_mapping, index);
> -	if (IS_ERR(folio))
> +	if ((!folio) || (IS_ERR(folio)))
>  		return 0;
>  
>  	size = folio_size(folio);

I was able to test the patch with the case from the Debian bugreport
and seems to resolve the issue. Even if late, as Greg just queued up
already:

Tested-by: Salvatore Bonaccorso <carnil@debian.org>

I wonder, but cannot(!) confirm, might there have been other such
backports in 6.1.y which are incorrect? At least it does not seem the
case so far for other uses of checking IS_ERR(folio) in v6.1.72 where
were using filemap_get_folio. So I guess the other cases are all okay.
In v6.1.72 finding those:

block/partitions/core.c-
block/partitions/core.c-        folio = read_mapping_folio(mapping, n >> PAGE_SECTORS_SHIFT, NULL);
block/partitions/core.c:        if (IS_ERR(folio))
--
drivers/scsi/scsicam.c-
drivers/scsi/scsicam.c- folio = read_mapping_folio(mapping, 0, NULL);
drivers/scsi/scsicam.c: if (IS_ERR(folio))
--
fs/ceph/addr.c-
fs/ceph/addr.c- folio = read_mapping_folio(inode->i_mapping, 0, file);
fs/ceph/addr.c: if (IS_ERR(folio)) {
--
fs/erofs/data.c-                folio = read_cache_folio(mapping, index, NULL, NULL);
fs/erofs/data.c-                memalloc_nofs_restore(nofs_flag);
fs/erofs/data.c:                if (IS_ERR(folio))
--
fs/ext2/dir.c-  struct folio *folio = read_mapping_folio(mapping, n, NULL);
fs/ext2/dir.c-
fs/ext2/dir.c:  if (IS_ERR(folio))
--
fs/smb/client/cifsfs.c-
fs/smb/client/cifsfs.c- folio = filemap_get_folio(inode->i_mapping, index);
fs/smb/client/cifsfs.c: if (IS_ERR(folio))
--
fs/verity/enable.c-                     page_cache_sync_ra(&ractl, remaining_pages);
fs/verity/enable.c-             folio = read_cache_folio(ractl.mapping, index, NULL, file);
fs/verity/enable.c:             if (IS_ERR(folio))
--
mm/filemap.c-
mm/filemap.c-   folio = do_read_cache_folio(mapping, index, filler, file, gfp);
mm/filemap.c:   if (IS_ERR(folio))
--
mm/shmem.c-     huge_gfp = limit_gfp_mask(huge_gfp, gfp);
mm/shmem.c-     folio = shmem_alloc_and_acct_folio(huge_gfp, inode, index, true);
mm/shmem.c:     if (IS_ERR(folio)) {
--
mm/shmem.c-             folio = shmem_alloc_and_acct_folio(gfp, inode, index, false);
mm/shmem.c-     }
mm/shmem.c:     if (IS_ERR(folio)) {

where only fs/smb/client/cifsfs.c was using filemap_get_folio() in this context.

Regards,
Salvatore