Re: [PATCH 1/3] smb: client: do not defer close open handles to deleted files

From: Meetakshi Setiya <meetakshisetiyaoss@gmail.com>
To: Steve French <smfrench@gmail.com>
Cc: Shyam Prasad N <nspmangalore@gmail.com>,
	Steve French <sfrench@samba.org>,
	 Paulo Alcantara <pc@manguebit.com>,
	ronnie sahlberg <ronniesahlberg@gmail.com>,
	 Shyam Prasad N <sprasad@microsoft.com>,
	Tom Talpey <tom@talpey.com>,  CIFS <linux-cifs@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	 samba-technical <samba-technical@lists.samba.org>,
	Bharath SM <bharathsm.hsk@gmail.com>,
	 Meetakshi Setiya <msetiya@microsoft.com>
Subject: Re: [PATCH 1/3] smb: client: do not defer close open handles to deleted files
Date: Wed, 21 Feb 2024 10:14:34 +0530	[thread overview]
Message-ID: <CAFTVevU94O1NEJ1XzrUSDoC7b4=W=wciqRWX8LoB1X1=AUNZfA@mail.gmail.com> (raw)
In-Reply-To: <CAH2r5mumpNgm62mSFQmtMANm-njH1VJsv4ZT50Cww9dTCed3XQ@mail.gmail.com>

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

PFA the follow up patch that uses a boolean field status_file_deleted
instead of a bitmap to mark open handles for files that have been deleted.
Additionally, if SMB2 query info response is received with the DeletePending
flag set to true for a client, mark the corresponding open file handles
in this case too.

Thanks
Meetakshi

On Sun, Feb 11, 2024 at 2:34 AM Steve French <smfrench@gmail.com> wrote:
>
> I lean toward using the existing attr field since it presumably could be set remotely and best to be safe and consistent and also uses less space
>
> ATTR_DELETE_ON_CLOSE
>
> On Sat, Feb 10, 2024, 11:50 Shyam Prasad N <nspmangalore@gmail.com> wrote:
>>
>> On Sat, Feb 10, 2024 at 11:29 AM Steve French <smfrench@gmail.com> wrote:
>> >
>> > could we make the "file_is_deleted" a boolean instead of using up more
>> > space making it an int?
>>
>> Meetakshi initially had it as a bool. I asked her to make it a bitmap,
>> just in case there can be other such flags that are needed in the
>> future.
>>
>> >
>> > Alternatively - would it be possible to simply look at the file
>> > attributes to see if it was marked as deleted (ie we should already be
>> > setting ATTR_DELETE_ON_CLOSE)
>>
>> It does not look like we use this attr anywhere today. (Am I missing something?)
>> Also, it looks like a flag that SMB uses in requests and responses.
>> I feel that it's better to keep a different flag for this purpose.
>>
>> >
>> > On Fri, Feb 9, 2024 at 7:17 AM <meetakshisetiyaoss@gmail.com> wrote:
>> > >
>> > > From: Meetakshi Setiya <msetiya@microsoft.com>
>> > >
>> > > When a file/dentry has been deleted before closing all its open handles,
>> > > currently, closing them can add them to the deferred close list. This can
>> > > lead to problems in creating file with the same name when the file is
>> > > re-created before the deferred close completes. This issue was seen while
>> > > reusing a client's already existing lease on a file for compound operations
>> > > and xfstest 591 failed because of the deferred close handle that remained
>> > > valid even after the file was deleted and was being reused to create a file
>> > > with the same name. The server in this case returns an error on open with
>> > > STATUS_DELETE_PENDING. Recreating the file would fail till the deferred
>> > > handles are closed (duration specified in closetimeo).
>> > >
>> > > This patch fixes the issue by flagging all open handles for the deleted
>> > > file (file path to be precise) with FILE_DELETED at the end of the unlink
>> > > operation. A new field cflags has been introduced in the cifsFileInfo
>> > > structure to set the FILE_DELETED flag without interfering with the f_flags
>> > > field. This cflags field could be useful in the future for setting more
>> > > flags specific to cfile.
>> > > When doing close in cifs_close for each of these handles, check the
>> > > FILE_DELETED flag and do not defer close these handles if the corresponding
>> > > filepath has been deleted.
>> > >
>> > > Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
>> > > ---
>> > >  fs/smb/client/cifsglob.h  |  3 +++
>> > >  fs/smb/client/cifsproto.h |  4 ++++
>> > >  fs/smb/client/file.c      |  3 ++-
>> > >  fs/smb/client/inode.c     |  2 ++
>> > >  fs/smb/client/misc.c      | 22 ++++++++++++++++++++++
>> > >  5 files changed, 33 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>> > > index 16befff4cbb4..f6b4acdcdeb3 100644
>> > > --- a/fs/smb/client/cifsglob.h
>> > > +++ b/fs/smb/client/cifsglob.h
>> > > @@ -1398,6 +1398,8 @@ struct cifs_fid_locks {
>> > >         struct list_head locks;         /* locks held by fid above */
>> > >  };
>> > >
>> > > +#define FILE_DELETED 00000001
>> > > +
>> > >  struct cifsFileInfo {
>> > >         /* following two lists are protected by tcon->open_file_lock */
>> > >         struct list_head tlist; /* pointer to next fid owned by tcon */
>> > > @@ -1413,6 +1415,7 @@ struct cifsFileInfo {
>> > >         struct dentry *dentry;
>> > >         struct tcon_link *tlink;
>> > >         unsigned int f_flags;
>> > > +       unsigned int cflags;    /* flags for this file */
>> > >         bool invalidHandle:1;   /* file closed via session abend */
>> > >         bool swapfile:1;
>> > >         bool oplock_break_cancelled:1;
>> > > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>> > > index a841bf4967fa..f995b766b177 100644
>> > > --- a/fs/smb/client/cifsproto.h
>> > > +++ b/fs/smb/client/cifsproto.h
>> > > @@ -296,6 +296,10 @@ extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
>> > >
>> > >  extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon *cifs_tcon,
>> > >                                 const char *path);
>> > > +
>> > > +extern void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon
>> > > +                               *cifs_tcon, const char *path);
>> > > +
>> > >  extern struct TCP_Server_Info *
>> > >  cifs_get_tcp_session(struct smb3_fs_context *ctx,
>> > >                      struct TCP_Server_Info *primary_server);
>> > > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
>> > > index b75282c204da..91cf4d2df4de 100644
>> > > --- a/fs/smb/client/file.c
>> > > +++ b/fs/smb/client/file.c
>> > > @@ -483,6 +483,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>> > >         cfile->uid = current_fsuid();
>> > >         cfile->dentry = dget(dentry);
>> > >         cfile->f_flags = file->f_flags;
>> > > +       cfile->cflags = 0;
>> > >         cfile->invalidHandle = false;
>> > >         cfile->deferred_close_scheduled = false;
>> > >         cfile->tlink = cifs_get_tlink(tlink);
>> > > @@ -1085,7 +1086,7 @@ int cifs_close(struct inode *inode, struct file *file)
>> > >                 if ((cifs_sb->ctx->closetimeo && cinode->oplock == CIFS_CACHE_RHW_FLG)
>> > >                     && cinode->lease_granted &&
>> > >                     !test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
>> > > -                   dclose) {
>> > > +                   dclose && !(cfile->cflags & FILE_DELETED)) {
>> > >                         if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode->flags)) {
>> > >                                 inode_set_mtime_to_ts(inode,
>> > >                                                       inode_set_ctime_current(inode));
>> > > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
>> > > index d02f8ba29cb5..8121b5b1aa22 100644
>> > > --- a/fs/smb/client/inode.c
>> > > +++ b/fs/smb/client/inode.c
>> > > @@ -1900,6 +1900,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
>> > >         cifs_inode = CIFS_I(dir);
>> > >         CIFS_I(dir)->time = 0;  /* force revalidate of dir as well */
>> > >  unlink_out:
>> > > +       if (rc == 0)
>> > > +               cifs_mark_open_handles_for_deleted_file(tcon, full_path);
>> > >         free_dentry_path(page);
>> > >         kfree(attrs);
>> > >         free_xid(xid);
>> > > diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
>> > > index 0748d7b757b9..8e46dee1a36c 100644
>> > > --- a/fs/smb/client/misc.c
>> > > +++ b/fs/smb/client/misc.c
>> > > @@ -853,6 +853,28 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
>> > >         free_dentry_path(page);
>> > >  }
>> > >
>> > > +/*
>> > > + * If a dentry has been deleted, all corresponding open handles should know that
>> > > + * so that we do not defer close them.
>> > > + */
>> > > +void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon *tcon,
>> > > +                                            const char *path)
>> > > +{
>> > > +       struct cifsFileInfo *cfile;
>> > > +       void *page;
>> > > +       const char *full_path;
>> > > +
>> > > +       page = alloc_dentry_path();
>> > > +       spin_lock(&tcon->open_file_lock);
>> > > +       list_for_each_entry(cfile, &tcon->openFileList, tlist) {
>> > > +               full_path = build_path_from_dentry(cfile->dentry, page);
>> > > +               if (strcmp(full_path, path) == 0)
>> > > +                       cfile->cflags |= FILE_DELETED;
>> > > +       }
>> > > +       spin_unlock(&tcon->open_file_lock);
>> > > +       free_dentry_path(page);
>> > > +}
>> > > +
>> > >  /* parses DFS referral V3 structure
>> > >   * caller is responsible for freeing target_nodes
>> > >   * returns:
>> > > --
>> > > 2.39.2
>> > >
>> > >
>> >
>> >
>> > --
>> > Thanks,
>> >
>> > Steve
>>
>>
>>
>> --
>> Regards,
>> Shyam

[-- Attachment #2: 0001-smb-client-do-not-defer-close-open-handles-to-delete.patch --]
[-- Type: application/octet-stream, Size: 8053 bytes --]

From 0767c1bd3b25559b466e2ad5069a2b616bd5d982 Mon Sep 17 00:00:00 2001
From: Meetakshi Setiya <msetiya@microsoft.com>
Date: Fri, 9 Feb 2024 05:13:22 -0500
Subject: [PATCH 1/3] smb: client: do not defer close open handles to deleted
 files

When a file/dentry has been deleted before closing all its open
handles, currently, closing them can add them to the deferred
close list. This can lead to problems in creating file with the
same name when the file is re-created before the deferred close
completes. This issue was seen while reusing a client's already
existing lease on a file for compound operations and xfstest 591
failed because of the deferred close handle that remained valid
even after the file was deleted and was being reused to create a
file with the same name. The server in this case returns an error
on open with STATUS_DELETE_PENDING. Recreating the file would
fail till the deferred handles are closed (duration specified in
closetimeo).

This patch fixes the issue by flagging all open handles for the
deleted file (file path to be precise) by setting
status_file_deleted to true in the cifsFileInfo structure. As per
the information classes specified in MS-FSCC, SMB2 query info
response from the server has a DeletePending field, set to true
to indicate that deletion has been requested on that file. If
this is the case, flag the open handles for this file too.

When doing close in cifs_close for each of these handles, check the
value of this boolean field and do not defer close these handles
if the corresponding filepath has been deleted.

Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/cifsglob.h  |  1 +
 fs/smb/client/cifsproto.h |  4 ++++
 fs/smb/client/file.c      |  3 ++-
 fs/smb/client/inode.c     | 23 ++++++++++++++++++-----
 fs/smb/client/misc.c      | 22 ++++++++++++++++++++++
 5 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 16befff4cbb4..d79b59c3b50c 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1416,6 +1416,7 @@ struct cifsFileInfo {
 	bool invalidHandle:1;	/* file closed via session abend */
 	bool swapfile:1;
 	bool oplock_break_cancelled:1;
+	bool status_file_deleted:1; /* file has been deleted */
 	unsigned int oplock_epoch; /* epoch from the lease break */
 	__u32 oplock_level; /* oplock/lease level from the lease break */
 	int count;
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index a841bf4967fa..f995b766b177 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -296,6 +296,10 @@ extern void cifs_close_all_deferred_files(struct cifs_tcon *cifs_tcon);
 
 extern void cifs_close_deferred_file_under_dentry(struct cifs_tcon *cifs_tcon,
 				const char *path);
+
+extern void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon
+				*cifs_tcon, const char *path);
+
 extern struct TCP_Server_Info *
 cifs_get_tcp_session(struct smb3_fs_context *ctx,
 		     struct TCP_Server_Info *primary_server);
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index b75282c204da..46951f403d31 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -483,6 +483,7 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
 	cfile->uid = current_fsuid();
 	cfile->dentry = dget(dentry);
 	cfile->f_flags = file->f_flags;
+	cfile->status_file_deleted = false;
 	cfile->invalidHandle = false;
 	cfile->deferred_close_scheduled = false;
 	cfile->tlink = cifs_get_tlink(tlink);
@@ -1085,7 +1086,7 @@ int cifs_close(struct inode *inode, struct file *file)
 		if ((cifs_sb->ctx->closetimeo && cinode->oplock == CIFS_CACHE_RHW_FLG)
 		    && cinode->lease_granted &&
 		    !test_bit(CIFS_INO_CLOSE_ON_LOCK, &cinode->flags) &&
-		    dclose) {
+		    dclose && !(cfile->status_file_deleted)) {
 			if (test_and_clear_bit(CIFS_INO_MODIFIED_ATTR, &cinode->flags)) {
 				inode_set_mtime_to_ts(inode,
 						      inode_set_ctime_current(inode));
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index d02f8ba29cb5..18ba40340623 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -807,7 +807,7 @@ bool cifs_reparse_point_to_fattr(struct cifs_sb_info *cifs_sb,
 
 static void cifs_open_info_to_fattr(struct cifs_fattr *fattr,
 				    struct cifs_open_info_data *data,
-				    struct super_block *sb)
+				    struct super_block *sb, const char *full_path)
 {
 	struct smb2_file_all_info *info = &data->fi;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
@@ -815,8 +815,10 @@ static void cifs_open_info_to_fattr(struct cifs_fattr *fattr,
 
 	memset(fattr, 0, sizeof(*fattr));
 	fattr->cf_cifsattrs = le32_to_cpu(info->Attributes);
-	if (info->DeletePending)
+	if (info->DeletePending) {
 		fattr->cf_flags |= CIFS_FATTR_DELETE_PENDING;
+		cifs_mark_open_handles_for_deleted_file(tcon, full_path);
+	}
 
 	if (info->LastAccessTime)
 		fattr->cf_atime = cifs_NTtimeToUnix(info->LastAccessTime);
@@ -893,6 +895,9 @@ cifs_get_file_info(struct file *filp)
 	struct cifsFileInfo *cfile = filp->private_data;
 	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
 	struct TCP_Server_Info *server = tcon->ses->server;
+	struct dentry *dentry = filp->f_path.dentry;
+	void *page = alloc_dentry_path();
+	const unsigned char *path;
 
 	if (!server->ops->query_file_info)
 		return -ENOSYS;
@@ -907,7 +912,12 @@ cifs_get_file_info(struct file *filp)
 			data.symlink = true;
 			data.reparse.tag = IO_REPARSE_TAG_SYMLINK;
 		}
-		cifs_open_info_to_fattr(&fattr, &data, inode->i_sb);
+		path = build_path_from_dentry(dentry, page);
+		if (IS_ERR(path)) {
+			free_dentry_path(page);
+			return PTR_ERR(path);
+		}
+		cifs_open_info_to_fattr(&fattr, &data, inode->i_sb, path);
 		break;
 	case -EREMOTE:
 		cifs_create_junction_fattr(&fattr, inode->i_sb);
@@ -937,6 +947,7 @@ cifs_get_file_info(struct file *filp)
 	rc = cifs_fattr_to_inode(inode, &fattr);
 cgfi_exit:
 	cifs_free_open_info(&data);
+	free_dentry_path(page);
 	free_xid(xid);
 	return rc;
 }
@@ -1115,7 +1126,7 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
 	if (tcon->posix_extensions)
 		smb311_posix_info_to_fattr(fattr, data, sb);
 	else
-		cifs_open_info_to_fattr(fattr, data, sb);
+		cifs_open_info_to_fattr(fattr, data, sb, full_path);
 out:
 	fattr->cf_cifstag = data->reparse.tag;
 	free_rsp_buf(rsp_buftype, rsp_iov.iov_base);
@@ -1169,7 +1180,7 @@ static int cifs_get_fattr(struct cifs_open_info_data *data,
 			rc = reparse_info_to_fattr(data, sb, xid, tcon,
 						   full_path, fattr);
 		} else {
-			cifs_open_info_to_fattr(fattr, data, sb);
+			cifs_open_info_to_fattr(fattr, data, sb, full_path);
 		}
 		break;
 	case -EREMOTE:
@@ -1900,6 +1911,8 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
 	cifs_inode = CIFS_I(dir);
 	CIFS_I(dir)->time = 0;	/* force revalidate of dir as well */
 unlink_out:
+	if (rc == 0)
+		cifs_mark_open_handles_for_deleted_file(tcon, full_path);
 	free_dentry_path(page);
 	kfree(attrs);
 	free_xid(xid);
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 0748d7b757b9..4ea18dd7b1a8 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -853,6 +853,28 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
 	free_dentry_path(page);
 }
 
+/*
+ * If a dentry has been deleted, all corresponding open handles should know that
+ * so that we do not defer close them.
+ */
+void cifs_mark_open_handles_for_deleted_file(struct cifs_tcon *tcon,
+					     const char *path)
+{
+	struct cifsFileInfo *cfile;
+	void *page;
+	const char *full_path;
+
+	page = alloc_dentry_path();
+	spin_lock(&tcon->open_file_lock);
+	list_for_each_entry(cfile, &tcon->openFileList, tlist) {
+		full_path = build_path_from_dentry(cfile->dentry, page);
+		if (strcmp(full_path, path) == 0)
+			cfile->status_file_deleted = true;
+	}
+	spin_unlock(&tcon->open_file_lock);
+	free_dentry_path(page);
+}
+
 /* parses DFS referral V3 structure
  * caller is responsible for freeing target_nodes
  * returns:
-- 
2.39.2

