Re: [PATCH v4 63/68] cifs: Support fscache indexing rewrite (untested)

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2022-01-07 at 21:52 +0000, David Howells wrote:
> This patch isn't quite right and needs a fix.  The attached patch fixes it.
> I'll fold that in and post a v5 of this patch.
> 
> David
> ---
> cifs: Fix the fscache cookie management
> 
> Fix the fscache cookie management in cifs in the following ways:
> 
>  (1) The cookie should be released in cifs_evict_inode() after it has been
>      unused from being pinned by cifs_set_page_dirty().
> 
>  (2) The cookie shouldn't be released in cifsFileInfo_put_final().  That's
>      dealing with closing a file, not getting rid of an inode.  The cookie
>      needs to persist beyond the last file close because writepages may
>      happen after closure.
> 
>  (3) The cookie needs to be used in cifs_atomic_open() to match
>      cifs_open().  As yet unknown files being opened for writing seem to go
>      by the former route rather than the latter.
> 
> This can be triggered by something like:
> 
>         # systemctl start cachefilesd
>         # mount //carina/test /xfstest.test -o user=shares,pass=xxxx.fsc
>         # bash 5</xfstest.test/bar
>         # echo hello >/xfstest.test/bar
> 
> The key is to open the file R/O and then open it R/W and write to it whilst
> it's still open for R/O.  A cookie isn't acquired if it's just opened R/W
> as it goes through the atomic_open method rather than the open method.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>  fs/cifs/cifsfs.c |    8 ++++++++
>  fs/cifs/dir.c    |    4 ++++
>  fs/cifs/file.c   |    2 --
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index d3f3acf340f1..26cf2193c9a2 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -398,6 +398,7 @@ cifs_evict_inode(struct inode *inode)
>  	truncate_inode_pages_final(&inode->i_data);
>  	if (inode->i_state & I_PINNING_FSCACHE_WB)
>  		cifs_fscache_unuse_inode_cookie(inode, true);
> +	cifs_fscache_release_inode_cookie(inode);
>  	clear_inode(inode);
>  }
>  
> @@ -722,6 +723,12 @@ static int cifs_show_stats(struct seq_file *s, struct dentry *root)
>  }
>  #endif
>  
> +static int cifs_write_inode(struct inode *inode, struct writeback_control *wbc)
> +{
> +	fscache_unpin_writeback(wbc, cifs_inode_cookie(inode));
> +	return 0;
> +}
> +
>  static int cifs_drop_inode(struct inode *inode)
>  {
>  	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> @@ -734,6 +741,7 @@ static int cifs_drop_inode(struct inode *inode)
>  static const struct super_operations cifs_super_ops = {
>  	.statfs = cifs_statfs,
>  	.alloc_inode = cifs_alloc_inode,
> +	.write_inode	= cifs_write_inode,
>  	.free_inode = cifs_free_inode,
>  	.drop_inode	= cifs_drop_inode,
>  	.evict_inode	= cifs_evict_inode,
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 6e8e7cc26ae2..6186824b366e 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -22,6 +22,7 @@
>  #include "cifs_unicode.h"
>  #include "fs_context.h"
>  #include "cifs_ioctl.h"
> +#include "fscache.h"
>  
>  static void
>  renew_parental_timestamps(struct dentry *direntry)
> @@ -509,6 +510,9 @@ cifs_atomic_open(struct inode *inode, struct dentry *direntry,
>  		rc = -ENOMEM;
>  	}
>  
> +	fscache_use_cookie(cifs_inode_cookie(file_inode(file)),
> +			   file->f_mode & FMODE_WRITE);
> +
>  out:
>  	cifs_put_tlink(tlink);
>  out_free_xid:
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index d872f6fe8e7d..44da7646f789 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -376,8 +376,6 @@ static void cifsFileInfo_put_final(struct cifsFileInfo *cifs_file)
>  	struct cifsLockInfo *li, *tmp;
>  	struct super_block *sb = inode->i_sb;
>  
> -	cifs_fscache_release_inode_cookie(inode);
> -
>  	/*
>  	 * Delete any outstanding lock records. We'll lose them when the file
>  	 * is closed anyway.
> 

Looks good.

Acked-by: Jeff Layton <jlayton@kernel.org>