| Message ID | 20231229143521.44880-2-meetakshisetiyaoss@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] smb: client: reuse file lease key in compound operations | expand |
meetakshisetiyaoss@gmail.com writes: > From: Meetakshi Setiya <msetiya@microsoft.com> > > There is a shortcoming in the current implementation of the file > lease mechanism exposed when the lease keys were attempted to be reused > for unlink, rename and set_path_size operations for a client. As per > MS-SMB2, lease keys are associated with the file name. Linux cifs client > maintains lease keys with the inode. If the file has any hardlinks, > it is possible that the lease for a file be wrongly reused for an > operation on the hardlink or vice versa. In these cases, the mentioned > compound operations fail with STATUS_INVALID_PARAMETER. > This patch adds a fallback to the old mechanism of not sending any > lease with these compound operations if the request with lease key fails > with STATUS_INVALID_PARAMETER. Resending the same request without lease > key should not hurt any functionality, but might impact performance > especially in cases where the error is not because of the usage of wrong > lease key and we might end up doing an extra roundtrip. What's the problem with checking ->i_nlink to decide whether reusing lease key?
On Fri, Dec 29, 2023 at 9:13 PM Paulo Alcantara <pc@manguebit.com> wrote: > > meetakshisetiyaoss@gmail.com writes: > > > From: Meetakshi Setiya <msetiya@microsoft.com> > > > > There is a shortcoming in the current implementation of the file > > lease mechanism exposed when the lease keys were attempted to be reused > > for unlink, rename and set_path_size operations for a client. As per > > MS-SMB2, lease keys are associated with the file name. Linux cifs client > > maintains lease keys with the inode. If the file has any hardlinks, > > it is possible that the lease for a file be wrongly reused for an > > operation on the hardlink or vice versa. In these cases, the mentioned > > compound operations fail with STATUS_INVALID_PARAMETER. > > This patch adds a fallback to the old mechanism of not sending any > > lease with these compound operations if the request with lease key fails > > with STATUS_INVALID_PARAMETER. Resending the same request without lease > > key should not hurt any functionality, but might impact performance > > especially in cases where the error is not because of the usage of wrong > > lease key and we might end up doing an extra roundtrip. > > What's the problem with checking ->i_nlink to decide whether reusing > lease key? As per the discussion with Tom on the previous version of the changes, I conferred with Shyam and Steve about possible workarounds and this seemed like a choice which did the job without much perf drawbacks and code changes. One highlighted difference between the two could be that in the previous version, lease would not be reused for any file with hardlinks at all, even though the inode may hold the correct lease for that particular file. The current changes would take care of this by sending the lease at least once, irrespective of the number of hardlinks. Thanks Meetakshi
Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes: > As per the discussion with Tom on the previous version of the changes, I > conferred with Shyam and Steve about possible workarounds and this seemed like a > choice which did the job without much perf drawbacks and code changes. One > highlighted difference between the two could be that in the previous > version, lease > would not be reused for any file with hardlinks at all, even though the inode > may hold the correct lease for that particular file. The current changes > would take care of this by sending the lease at least once, irrespective of the > number of hardlinks. Thanks for the explanation. However, the code change size is no excuse for providing workarounds rather than the actual fix. A possible way to handle such case would be keeping a list of pathname:lease_key pairs inside the inode, so in smb2_compound_op() you could look up the lease key by using @dentry. I'm not sure if there's a better way to handle it as I haven't looked into it further.
On 1/3/2024 9:37 AM, Paulo Alcantara wrote: > Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes: > >> As per the discussion with Tom on the previous version of the changes, I >> conferred with Shyam and Steve about possible workarounds and this seemed like a >> choice which did the job without much perf drawbacks and code changes. One >> highlighted difference between the two could be that in the previous >> version, lease >> would not be reused for any file with hardlinks at all, even though the inode >> may hold the correct lease for that particular file. The current changes >> would take care of this by sending the lease at least once, irrespective of the >> number of hardlinks. > > Thanks for the explanation. However, the code change size is no excuse > for providing workarounds rather than the actual fix. I have to agree. And it really isn't much of a workaround either. > A possible way to handle such case would be keeping a list of > pathname:lease_key pairs inside the inode, so in smb2_compound_op() you > could look up the lease key by using @dentry. I'm not sure if there's a > better way to handle it as I haven't looked into it further. A list would also allow for better handling of lease revocation. It seems to me this approach basically discards the original lease, putting the client's cached data at risk, no? And what happens if EINVAL comes back again, or the connection breaks? Is this a recoverable situation? Also, what's up with the xfstest the robot mailed about? Tom.
Tom Talpey <tom@talpey.com> writes: > On 1/3/2024 9:37 AM, Paulo Alcantara wrote: >> A possible way to handle such case would be keeping a list of >> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you >> could look up the lease key by using @dentry. I'm not sure if there's a >> better way to handle it as I haven't looked into it further. > > A list would also allow for better handling of lease revocation. It's also worth mentioning that we could also map the dentry directly to lease key, so no need to store pathnames inside the inode. > It seems to me this approach basically discards the original lease, > putting the client's cached data at risk, no? And what happens if > EINVAL comes back again, or the connection breaks? Is this a > recoverable situation? These are really good points and would need to be investigated before coming up with an implementation.
On Fri, Jan 5, 2024 at 4:39 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Tom Talpey <tom@talpey.com> writes: > > > On 1/3/2024 9:37 AM, Paulo Alcantara wrote: > >> A possible way to handle such case would be keeping a list of > >> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you > >> could look up the lease key by using @dentry. I'm not sure if there's a > >> better way to handle it as I haven't looked into it further. > > > > A list would also allow for better handling of lease revocation. > > It's also worth mentioning that we could also map the dentry directly to > lease key, so no need to store pathnames inside the inode. We were discussing just this yesterday. That we don't really need path names as the key. It could be the dentry. > > > It seems to me this approach basically discards the original lease, > > putting the client's cached data at risk, no? And what happens if > > EINVAL comes back again, or the connection breaks? Is this a > > recoverable situation? > > These are really good points and would need to be investigated before > coming up with an implementation.
On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@talpey.com> wrote: > > On 1/3/2024 9:37 AM, Paulo Alcantara wrote: > > Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes: > > > >> As per the discussion with Tom on the previous version of the changes, I > >> conferred with Shyam and Steve about possible workarounds and this seemed like a > >> choice which did the job without much perf drawbacks and code changes. One > >> highlighted difference between the two could be that in the previous > >> version, lease > >> would not be reused for any file with hardlinks at all, even though the inode > >> may hold the correct lease for that particular file. The current changes > >> would take care of this by sending the lease at least once, irrespective of the > >> number of hardlinks. > > > > Thanks for the explanation. However, the code change size is no excuse > > for providing workarounds rather than the actual fix. > > I have to agree. And it really isn't much of a workaround either. > The original problem, i.e. compound operations like unlink/rename/setsize not sending a lease key is very prevalent on the field. Unfortunately, fixing that exposed this problem with hard links. So Steve suggested getting this fix to a shape where it's fixing the original problem, even if it means that it does not fix it for the case of where there are open handles to multiple hard links to the same file. Only thing we need to be careful of is that it does not make things worse for other workloads. > > A possible way to handle such case would be keeping a list of > > pathname:lease_key pairs inside the inode, so in smb2_compound_op() you > > could look up the lease key by using @dentry. I'm not sure if there's a > > better way to handle it as I haven't looked into it further. > This seems like a reasonable change to make. That will make sure that we stick to what the protocol recommends. I'm not sure that this change will be a simple one. There could be several places where we make an assumption that the lease is associated with an inode, and not a link. And I'm not yet fully convinced that the spec itself is doing the right thing by tying the lease with the link, rather than the file. Shouldn't the lease protect the data of the file, and not the link itself? If opening two links to the same file with two different lease keys end up breaking each other's leases, what's the point? > A list would also allow for better handling of lease revocation. > It seems to me this approach basically discards the original lease, > putting the client's cached data at risk, no? And what happens if > EINVAL comes back again, or the connection breaks? Is this a > recoverable situation? > > Also, what's up with the xfstest the robot mailed about? Meetakshi is investigating this one. Initial investigations led us to believe that this is related to deferred closes. The failing tests do show that the close is getting delayed, and that setting closetimeo=0 causes the test to pass. However, it is not clear why the test started failing only now. We'll have the answers soon. > > Tom.
Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical: > On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@talpey.com> wrote: >> >> On 1/3/2024 9:37 AM, Paulo Alcantara wrote: >>> Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes: >>> >>>> As per the discussion with Tom on the previous version of the changes, I >>>> conferred with Shyam and Steve about possible workarounds and this seemed like a >>>> choice which did the job without much perf drawbacks and code changes. One >>>> highlighted difference between the two could be that in the previous >>>> version, lease >>>> would not be reused for any file with hardlinks at all, even though the inode >>>> may hold the correct lease for that particular file. The current changes >>>> would take care of this by sending the lease at least once, irrespective of the >>>> number of hardlinks. >>> >>> Thanks for the explanation. However, the code change size is no excuse >>> for providing workarounds rather than the actual fix. >> >> I have to agree. And it really isn't much of a workaround either. >> > > The original problem, i.e. compound operations like > unlink/rename/setsize not sending a lease key is very prevalent on the > field. > Unfortunately, fixing that exposed this problem with hard links. > So Steve suggested getting this fix to a shape where it's fixing the > original problem, even if it means that it does not fix it for the > case of where there are open handles to multiple hard links to the > same file. > Only thing we need to be careful of is that it does not make things > worse for other workloads. > >>> A possible way to handle such case would be keeping a list of >>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you >>> could look up the lease key by using @dentry. I'm not sure if there's a >>> better way to handle it as I haven't looked into it further. >> > > This seems like a reasonable change to make. That will make sure that > we stick to what the protocol recommends. > I'm not sure that this change will be a simple one. There could be > several places where we make an assumption that the lease is > associated with an inode, and not a link. > > And I'm not yet fully convinced that the spec itself is doing the > right thing by tying the lease with the link, rather than the file. > Shouldn't the lease protect the data of the file, and not the link > itself? If opening two links to the same file with two different lease > keys end up breaking each other's leases, what's the point? I guess the reason for making the lease key per path on the client is that the client can't know about possible hardlinks before opening the file, but that open wants to use a leasekey... Or a "stat" open that won't any lease needs to be done first, which doubles the roundtrip for every open. And hard links are not that common... Maybe choosing und using a new leasekey would be the way to start with and when a hardlink is detected the open on the hardlink is closed again and retried with the former lease key, which would also upgrade it again. But sharing the handle lease for two pathnames seems wrong, as the idea of the handle lease is to cache the patchname on the client. While sharing the RW lease between two hardlinks would be desired. metze
Hi Metze, On Fri, Jan 5, 2024 at 3:30 PM Stefan Metzmacher <metze@samba.org> wrote: > > Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical: > > On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@talpey.com> wrote: > >> > >> On 1/3/2024 9:37 AM, Paulo Alcantara wrote: > >>> Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes: > >>> > >>>> As per the discussion with Tom on the previous version of the changes, I > >>>> conferred with Shyam and Steve about possible workarounds and this seemed like a > >>>> choice which did the job without much perf drawbacks and code changes. One > >>>> highlighted difference between the two could be that in the previous > >>>> version, lease > >>>> would not be reused for any file with hardlinks at all, even though the inode > >>>> may hold the correct lease for that particular file. The current changes > >>>> would take care of this by sending the lease at least once, irrespective of the > >>>> number of hardlinks. > >>> > >>> Thanks for the explanation. However, the code change size is no excuse > >>> for providing workarounds rather than the actual fix. > >> > >> I have to agree. And it really isn't much of a workaround either. > >> > > > > The original problem, i.e. compound operations like > > unlink/rename/setsize not sending a lease key is very prevalent on the > > field. > > Unfortunately, fixing that exposed this problem with hard links. > > So Steve suggested getting this fix to a shape where it's fixing the > > original problem, even if it means that it does not fix it for the > > case of where there are open handles to multiple hard links to the > > same file. > > Only thing we need to be careful of is that it does not make things > > worse for other workloads. > > > >>> A possible way to handle such case would be keeping a list of > >>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you > >>> could look up the lease key by using @dentry. I'm not sure if there's a > >>> better way to handle it as I haven't looked into it further. > >> > > > > This seems like a reasonable change to make. That will make sure that > > we stick to what the protocol recommends. > > I'm not sure that this change will be a simple one. There could be > > several places where we make an assumption that the lease is > > associated with an inode, and not a link. > > > > And I'm not yet fully convinced that the spec itself is doing the > > right thing by tying the lease with the link, rather than the file. > > Shouldn't the lease protect the data of the file, and not the link > > itself? If opening two links to the same file with two different lease > > keys end up breaking each other's leases, what's the point? > > I guess the reason for making the lease key per path on > the client is that the client can't know about possible hardlinks > before opening the file, but that open wants to use a leasekey... > Or a "stat" open that won't any lease needs to be done first, > which doubles the roundtrip for every open. > > And hard links are not that common... > That does makes sense. > Maybe choosing und using a new leasekey would be the > way to start with and when a hardlink is detected > the open on the hardlink is closed again and retried > with the former lease key, which would also upgrade it again. That would not work today, as the former lease key would be associated with the other hardlink. And would result in the server returning STATUS_INVALID_PARAMETER. > > But sharing the handle lease for two pathnames seems wrong, > as the idea of the handle lease is to cache the patchname on the client. > > While sharing the RW lease between two hardlinks would be desired. > > metze
Hi Shyam, >> Maybe choosing und using a new leasekey would be the >> way to start with and when a hardlink is detected >> the open on the hardlink is closed again and retried >> with the former lease key, which would also upgrade it again. > > That would not work today, as the former lease key would be associated > with the other hardlink. And would result in the server returning > STATUS_INVALID_PARAMETER. And that's the original problem you try to solve, correct? Then there's nothing we can do expect for using a dentry pased lease key and live with the fact that they don't allow write caching anymore. The RH state should still be granted to both lease keys... metze
On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <metze@samba.org> wrote: > > Hi Shyam, > > >> Maybe choosing und using a new leasekey would be the > >> way to start with and when a hardlink is detected > >> the open on the hardlink is closed again and retried > >> with the former lease key, which would also upgrade it again. > > > > That would not work today, as the former lease key would be associated > > with the other hardlink. And would result in the server returning > > STATUS_INVALID_PARAMETER. > > And that's the original problem you try to solve, correct? Correct. I thought you were proposing this as a solution. > > Then there's nothing we can do expect for using a dentry pased > lease key and live with the fact that they don't allow write caching > anymore. The RH state should still be granted to both lease keys... Yes. It's not ideal. But I guess we need to live with it. Thanks for the inputs. Steve/Paulo/Tom: What do you feel about fixing this in two phases? First, take Meetakshi's earlier patch, which would fix the problem of unnecessary lease breaks (and possible deadlock situation with the server) due to unlink/rename/setfilesize for files that do not have multiple hard links. i .e. during these operations, check if link count for the file is 1. Only if that is the case, send the lease key for the file. This would mean that the problem remains for files that have multiple hard links. But at least the hard link xfstest would pass. As a following patch, work on the full fix. i.e. maintain a list of lease keys for the file, keyed by the dentry. This patch would replace the cinode->lease_key with a map/list, lookup the correct lease from the list on usage. This would obviously remove the check for the link count done by the above patch. Reason being that we already have the first patch, and I'm not sure we'll be able to work on the second one soon enough. > > metze >
On Fri, Jan 5, 2024 at 4:58 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <metze@samba.org> wrote: > > > > Hi Shyam, > > > > >> Maybe choosing und using a new leasekey would be the > > >> way to start with and when a hardlink is detected > > >> the open on the hardlink is closed again and retried > > >> with the former lease key, which would also upgrade it again. > > > > > > That would not work today, as the former lease key would be associated > > > with the other hardlink. And would result in the server returning > > > STATUS_INVALID_PARAMETER. > > > > And that's the original problem you try to solve, correct? > Correct. I thought you were proposing this as a solution. > > > > Then there's nothing we can do expect for using a dentry pased > > lease key and live with the fact that they don't allow write caching > > anymore. The RH state should still be granted to both lease keys... > > Yes. It's not ideal. But I guess we need to live with it. > Thanks for the inputs. > > Steve/Paulo/Tom: What do you feel about fixing this in two phases? > > First, take Meetakshi's earlier patch, which would fix the problem of > unnecessary lease breaks (and possible deadlock situation with the > server) due to unlink/rename/setfilesize for files that do not have > multiple hard links. i > .e. during these operations, check if link count for the file is 1. > Only if that is the case, send the lease key for the file. This would > mean that the problem remains for files that have multiple hard links. > But at least the hard link xfstest would pass. Since this approach could be a huge performance degradation for some (albeit rare) workloads (e.g. if hardlinks exist for files, but such files are not opened by different names at the same time), I prefer the two phase approach that simply retries when we get invalid argument without the lease key (which has no risk since the current code just fails, and retry will "fix" the issue albeit not as good as being able to cache the second open) > As a following patch, work on the full fix. i.e. maintain a list of > lease keys for the file, keyed by the dentry. > This patch would replace the cinode->lease_key with a map/list, lookup > the correct lease from the list on usage. > This would obviously remove the check for the link count done by the > above patch. I don't like the idea of hurting caching for all hardlinked files as a hack, so for the longer term solution I prefer a solution that caches the dentry pointer with the lease key, although that brings up the obvious question of whether the dentry could be freed and reallocated in some deferred close cases and cause the lease key to be valid but us not to match it due to new dentry). I lean toward something like: 1) store the dentry for the lease key, not just the lease key in the cifs inode info (today we only store the lease key). We could store an array of lease key/dentry pairs but I worry that this would run the risk of use after free and/or lock contention bugs (and additional memory usage if a malicious app tried to open all hardlinked files) 2) if link count is greater than 1, check that the dentry matches when deciding whether to use the lease key (presumably we don't have to worry about it link count is 1)
Hi Here is why xfstest 591 must be failing when the lease key is being reused for the unlink compound operation and closetimeo is set to any value other than 0 (deferred close is supported). The splice_test program xfstest 591 uses calls unlink() at the end of the program to delete the file A without closing the file handle first. This file is thus marked for delete and the server returns STATUS_DELETE_PENDING. If the mount options have specified anything other than closetimeo=0 i.e. if we have enabled deferred closes, the open handle to this file (which was not closed by the application), would have deferred closed once the application ends. When the shell script runs the splice_test program again, the same handle from the previous run of the splice_test program - that was supposed to be closed because the file is deleted- is used by the next open to open file A since handle caching is still allowed for its lease. This leads to the error: No such file or directory which we see in the file 591.out.bad. This error was not observed when unlink operation broke leases because when the server issued a lease break to the client, it made the client flush its remaining writes/locks to the server and downgrade its lease from RWH to R. Since handle caching is not involved here anymore, the handle was also not reused anymore by the next open. Now that the patch has removed the lease break communication with the server, something similar to what happens when a client gets lease break notification might need to be done. One solution could be to flush all cached writes to the server and downgrade all leases for open handles to file A to R or 0 as soon as unlink is issued for the file A. In this case, even if some file handles are deferred closed, they would not be reused. A simple repro for this bug when the above patch 1/2 is applied (courtesy of @bharath): 1. Mount share with closetimeo=30 2. (shell1): tail -f /dev/null > myfile 3. (shell2): rm myfile 4. (shell1): ctrl+c to close myfile handle. This would be deferred closed 5. Touch myfile will fail for a period = closetimeo value Thanks Meetakshi On Sat, Jan 6, 2024 at 12:12 AM Steve French <smfrench@gmail.com> wrote: > > On Fri, Jan 5, 2024 at 4:58 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <metze@samba.org> wrote: > > > > > > Hi Shyam, > > > > > > >> Maybe choosing und using a new leasekey would be the > > > >> way to start with and when a hardlink is detected > > > >> the open on the hardlink is closed again and retried > > > >> with the former lease key, which would also upgrade it again. > > > > > > > > That would not work today, as the former lease key would be associated > > > > with the other hardlink. And would result in the server returning > > > > STATUS_INVALID_PARAMETER. > > > > > > And that's the original problem you try to solve, correct? > > Correct. I thought you were proposing this as a solution. > > > > > > Then there's nothing we can do expect for using a dentry pased > > > lease key and live with the fact that they don't allow write caching > > > anymore. The RH state should still be granted to both lease keys... > > > > Yes. It's not ideal. But I guess we need to live with it. > > Thanks for the inputs. > > > > Steve/Paulo/Tom: What do you feel about fixing this in two phases? > > > > First, take Meetakshi's earlier patch, which would fix the problem of > > unnecessary lease breaks (and possible deadlock situation with the > > server) due to unlink/rename/setfilesize for files that do not have > > multiple hard links. i > > .e. during these operations, check if link count for the file is 1. > > Only if that is the case, send the lease key for the file. This would > > mean that the problem remains for files that have multiple hard links. > > But at least the hard link xfstest would pass. > > Since this approach could be a huge performance degradation for some > (albeit rare) > workloads (e.g. if hardlinks exist for files, but such files are not opened by > different names at the same time), I prefer the two phase approach > that simply retries when we get invalid argument without the lease key > (which has no risk since the current code just fails, and retry will "fix" the > issue albeit not as good as being able to cache the second open) > > > As a following patch, work on the full fix. i.e. maintain a list of > > lease keys for the file, keyed by the dentry. > > This patch would replace the cinode->lease_key with a map/list, lookup > > the correct lease from the list on usage. > > This would obviously remove the check for the link count done by the > > above patch. > > I don't like the idea of hurting caching for all hardlinked files as a hack, > so for the longer term solution I prefer a solution that caches the > dentry pointer with the lease key, although that brings up the obvious > question of whether the dentry could be freed and reallocated > in some deferred close cases and cause the lease key to be valid > but us not to match it due to new dentry). > > I lean toward something like: > 1) store the dentry for the lease key, not just the lease key in the > cifs inode info (today we only store the lease key). > We could store an array of lease key/dentry pairs but I worry that > this would run the risk of use after free and/or lock contention bugs > (and additional memory usage if a malicious app tried to open all > hardlinked files) > 2) if link count is greater than 1, check that the dentry matches when > deciding whether to use the lease key (presumably > we don't have to worry about it link count is 1) > > -- > Thanks, > > Steve
Hi There was a pending fix to reuse lease key in compound operations on the smb client, essentially proposed to resolve a customer-reported bug. A scenario similar to what the client faced would be this: Imagine a file that has a lot of dirty pages to be written to the server. If rename/unlink/set path size compound operations are performed on this file, with the current implementation, we do not send the lease key. Resultantly, this would lead to the server assuming that the request came from a new client and it would send a lease break notification to the same client, on the same connection. This lease break can lead the client to consume several credits while writing its dirty pages to the server. A deadlock may arise when the server stops granting more credits to this connection and the deadlock would be lifted only when the lease timer on the server expires. The fix initially proposed just copied the lease key from the inode and passed it along to SMB2_open_init() from unlink, rename and set path size compound operations. Incidentally, that exposed a shortcoming in the smb client-side code. As per MS-SMB2, lease keys are associated with the file name. Linux cifs client maintains lease keys with the inode. If the file has any hardlinks, it is possible that the lease for a file be wrongly reused for an operation on the hardlink or vice versa. In these cases, the mentioned compound operations fail with STATUS_INVALID_PARAMETER. A simple, but hacky fix for the above, as per my discussion with Steve would be to have a two-phased approach and resend the compound op request again without the lease key if STATUS_INVALID_PARAMETER is received. This fix could be easily backported to older kernels since it would be pretty straightforward and does not involve a lot of code changes. Such a fallback mechanism should not hurt any functionality but might impact performance especially in cases where the error is not because of the usage of wrong lease key and we might end up doing an extra roundtrip. From what I know, use cases where two or more file+hardlinks are being used together at the same time are kind of rare, so we might not run into cases where resending requests in this fashion has to be performed often. I understand this is not a perfect or correct fix to the problem, but for the time being, it might help solve the customer reported issue I mentioned at the start and could also be backported readily. Since hurting caching for all hardlinked files is not ideal, I am already working on another fix that stores the dentry pointer with the lease key in the cinode object. From my discussion with Steve and Shyam, the fix proposed was this: Instead of having a list of dentries/key dentry pairs, we can store a dentry pointer in addition to the lease key in the cinode object. The dentry (as a proxy for filename/path) would be the one the lease key is associated with. There would be a reference counter to maintain the number of open handles for that file(path)/dentry. When a new file is created, not only its lease key, but its dentry too be set in the cinode object. Whenever there is a new handle opened to this dentry, the reference count to the leasekey/dentry in the cinode object would be increased. While reusing the lease key, check if dentry (from the request) matches the dentry stored in the cinode. If it does, copy the lease key, if it does not, do not use that lease key. When all file handles to a dentry have been closed, clear the dentry and lease key from the cinode object. This has the potential to fix the hardlink issue since dentries for different hardlinks would be different and one would not end up reusing lease from another. Some implementation details I am unsure about is should the open request for a file with mismatched dentry (which would not get to reuse the lease key) send no lease context at all, or create a new lease key and use that, or something else? I am writing this mail to seek advice/feedback on the situation. Any suggestions or better solutions to any of the issues mentioned in this mail are very much appreciated. Thanks Meetakshi On Fri, Jan 5, 2024 at 3:30 PM Stefan Metzmacher <metze@samba.org> wrote: > > Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical: > > On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@talpey.com> wrote: > >> > >> On 1/3/2024 9:37 AM, Paulo Alcantara wrote: > >>> Meetakshi Setiya <meetakshisetiyaoss@gmail.com> writes: > >>> > >>>> As per the discussion with Tom on the previous version of the changes, I > >>>> conferred with Shyam and Steve about possible workarounds and this seemed like a > >>>> choice which did the job without much perf drawbacks and code changes. One > >>>> highlighted difference between the two could be that in the previous > >>>> version, lease > >>>> would not be reused for any file with hardlinks at all, even though the inode > >>>> may hold the correct lease for that particular file. The current changes > >>>> would take care of this by sending the lease at least once, irrespective of the > >>>> number of hardlinks. > >>> > >>> Thanks for the explanation. However, the code change size is no excuse > >>> for providing workarounds rather than the actual fix. > >> > >> I have to agree. And it really isn't much of a workaround either. > >> > > > > The original problem, i.e. compound operations like > > unlink/rename/setsize not sending a lease key is very prevalent on the > > field. > > Unfortunately, fixing that exposed this problem with hard links. > > So Steve suggested getting this fix to a shape where it's fixing the > > original problem, even if it means that it does not fix it for the > > case of where there are open handles to multiple hard links to the > > same file. > > Only thing we need to be careful of is that it does not make things > > worse for other workloads. > > > >>> A possible way to handle such case would be keeping a list of > >>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you > >>> could look up the lease key by using @dentry. I'm not sure if there's a > >>> better way to handle it as I haven't looked into it further. > >> > > > > This seems like a reasonable change to make. That will make sure that > > we stick to what the protocol recommends. > > I'm not sure that this change will be a simple one. There could be > > several places where we make an assumption that the lease is > > associated with an inode, and not a link. > > > > And I'm not yet fully convinced that the spec itself is doing the > > right thing by tying the lease with the link, rather than the file. > > Shouldn't the lease protect the data of the file, and not the link > > itself? If opening two links to the same file with two different lease > > keys end up breaking each other's leases, what's the point? > > I guess the reason for making the lease key per path on > the client is that the client can't know about possible hardlinks > before opening the file, but that open wants to use a leasekey... > Or a "stat" open that won't any lease needs to be done first, > which doubles the roundtrip for every open. > > And hard links are not that common... > > Maybe choosing und using a new leasekey would be the > way to start with and when a hardlink is detected > the open on the hardlink is closed again and retried > with the former lease key, which would also upgrade it again. > > But sharing the handle lease for two pathnames seems wrong, > as the idea of the handle lease is to cache the patchname on the client. > > While sharing the RW lease between two hardlinks would be desired. > > metze
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c index 446433606a04..d3d7a4652a5b 100644 --- a/fs/smb/client/smb2inode.c +++ b/fs/smb/client/smb2inode.c @@ -121,6 +121,17 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon, } /* if there is an existing lease, reuse it */ + + /* + * note: files with hardlinks cause unexpected behaviour. As per MS-SMB2, + * lease keys are associated with the file name. We are maintaining lease keys + * with the inode on the client. If the file has hardlinks associated with it, + * it could be possible that the lease for a file be reused for an operation + * on the hardlink or vice versa. + * As a workaround, send request using an existing lease key and if the server + * returns STATUS_INVALID_PARAMETER, which maps to EINVAL, send the request + * again without the lease. + */ if (dentry) { inode = d_inode(dentry); cinode = CIFS_I(inode); @@ -907,10 +918,18 @@ int smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name, struct cifs_sb_info *cifs_sb, struct dentry *dentry) { - return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN, + int rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN, CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT, ACL_NO_MODE, NULL, &(int){SMB2_OP_DELETE}, 1, NULL, NULL, NULL, NULL, NULL, dentry); + if (rc == -EINVAL) { + cifs_dbg(FYI, "invalid lease key, resending request without lease"); + rc = smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN, + CREATE_DELETE_ON_CLOSE | OPEN_REPARSE_POINT, + ACL_NO_MODE, NULL, &(int){SMB2_OP_DELETE}, 1, + NULL, NULL, NULL, NULL, NULL, NULL); + } + return rc; } static int smb2_set_path_attr(const unsigned int xid, struct cifs_tcon *tcon, @@ -950,8 +969,14 @@ int smb2_rename_path(const unsigned int xid, drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb); cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile); - return smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb, + int rc = smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb, co, DELETE, SMB2_OP_RENAME, cfile, source_dentry); + if (rc == -EINVAL) { + cifs_dbg(FYI, "invalid lease key, resending request without lease"); + smb2_set_path_attr(xid, tcon, from_name, to_name, cifs_sb, + co, DELETE, SMB2_OP_RENAME, cfile, NULL); + } + return rc; } int smb2_create_hardlink(const unsigned int xid, @@ -979,11 +1004,20 @@ smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon, in_iov.iov_base = &eof; in_iov.iov_len = sizeof(eof); cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile); - return smb2_compound_op(xid, tcon, cifs_sb, full_path, + int rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, FILE_WRITE_DATA, FILE_OPEN, 0, ACL_NO_MODE, &in_iov, &(int){SMB2_OP_SET_EOF}, 1, cfile, NULL, NULL, NULL, NULL, dentry); + if (rc == -EINVAL) { + cifs_dbg(FYI, "invalid lease key, resending request without lease"); + rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, + FILE_WRITE_DATA, FILE_OPEN, + 0, ACL_NO_MODE, &in_iov, + &(int){SMB2_OP_SET_EOF}, 1, + cfile, NULL, NULL, NULL, NULL, NULL); + } + return rc; } int