| Message ID | 20210415152409.GA2286719@LEGION (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] cifs: remove unnecessary copies of tcon->crfid.fid | expand |
Hi, This is better I think. Muhammad Usama Anjum <musamaanjum@gmail.com> writes: > @@ -894,6 +891,10 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > > /* BB TBD check to see if oplock level check can be removed below */ > if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) { > + /* > + * caller expects this func to set the fid in crfid to valid > + * cached root, so increment the refcount. > + */ This comment is misleading. crfid variable doesn't exist anymore, and the kref_get() here is because of this commit: commit 2f94a3125b87 Author: Ronnie Sahlberg <lsahlber@redhat.com> Date: Thu Mar 28 11:20:02 2019 +1000 cifs: fix kref underflow in close_shroot() [...] --> This extra get() is only used to hold the structure until we get a lease --> break from the server at which point we will kref_put() it during lease --> processing. [...] When we queue a lease break, we usually get() the cifsFileInfo, but if that cifsFileInfo is backed by a cached_fid, the cached_fid isn't bumped. That commit was probably a work around for that. @Ronnie : struct cached_fid is starting to look very much like struct cifsFileInfo. I wonder why we couldn't use it, along with find_writable_file()/find_readable_file() to handle the caching. Alternatively, make cifsFileInfo use cached_fid (perhaps renaming it in the process, I don't know) Because I suspect a lot more issues will come up regarding cached_fid refcount and cifsFileInfo refcount going out of sync otherwise. Cheers,
I changed the comment to + /* + * See commit 2f94a3125b87. Increment the refcount when we + * get a lease for root, release it if lease break occurs + */ and added Aurelien's Reviewed-by. Let me know if you see any additional problems. On Sat, Apr 17, 2021 at 5:54 AM Aurélien Aptel <aaptel@suse.com> wrote: > > Hi, > > This is better I think. > > Muhammad Usama Anjum <musamaanjum@gmail.com> writes: > > @@ -894,6 +891,10 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > > > > /* BB TBD check to see if oplock level check can be removed below */ > > if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) { > > + /* > > + * caller expects this func to set the fid in crfid to valid > > + * cached root, so increment the refcount. > > + */ > > This comment is misleading. crfid variable doesn't exist anymore, and > the kref_get() here is because of this commit: > > commit 2f94a3125b87 > Author: Ronnie Sahlberg <lsahlber@redhat.com> > Date: Thu Mar 28 11:20:02 2019 +1000 > > cifs: fix kref underflow in close_shroot() > > [...] > --> This extra get() is only used to hold the structure until we get a lease > --> break from the server at which point we will kref_put() it during lease > --> processing. > [...] > > > > When we queue a lease break, we usually get() the cifsFileInfo, but if > that cifsFileInfo is backed by a cached_fid, the cached_fid isn't > bumped. That commit was probably a work around for that. > > @Ronnie : > > struct cached_fid is starting to look very much like struct > cifsFileInfo. I wonder why we couldn't use it, along with > find_writable_file()/find_readable_file() to handle the caching. > > Alternatively, make cifsFileInfo use cached_fid (perhaps renaming it in > the process, I don't know) > > Because I suspect a lot more issues will come up regarding cached_fid > refcount and cifsFileInfo refcount going out of sync otherwise. > > Cheers, > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) >
On Mon, 2021-04-19 at 18:39 -0500, Steve French wrote: > I changed the comment to > > + /* > + * See commit 2f94a3125b87. Increment the refcount when we > + * get a lease for root, release it if lease break occurs > + */ > > and added Aurelien's Reviewed-by. Let me know if you see any > additional problems. > Thank you so much!
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index caa5432a5ed1..797a20714ca1 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -848,11 +848,9 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, }; /* - * caller expects this func to set pfid to a valid - * cached root, so we copy the existing one and get a - * reference. + * caller expects this func to set the fid in crfid to valid + * cached root, so increment the refcount. */ - memcpy(pfid, tcon->crfid.fid, sizeof(*pfid)); kref_get(&tcon->crfid.refcount); mutex_unlock(&tcon->crfid.fid_mutex); @@ -885,7 +883,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); #endif /* CIFS_DEBUG2 */ - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); tcon->crfid.tcon = tcon; tcon->crfid.is_valid = true; tcon->crfid.dentry = dentry; @@ -894,6 +891,10 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, /* BB TBD check to see if oplock level check can be removed below */ if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) { + /* + * caller expects this func to set the fid in crfid to valid + * cached root, so increment the refcount. + */ kref_get(&tcon->crfid.refcount); tcon->crfid.has_lease = true; smb2_parse_contexts(server, o_rsp,
pfid is being set to tcon->crfid.fid and they are copied in each other multiple times. Remove the memcopy between same pointers - memory locations. Addresses-Coverity: ("Overlapped copy") Fixes: 9e81e8ff74b9 ("cifs: return cached_fid from open_shroot") Signed-off-by: Muhammad Usama Anjum <musamaanjum@gmail.com> --- Changes in V2: refcount increment is necessary. Don't remove it. Add and improve comments. fs/cifs/smb2ops.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)