On 3/22/2021 1:07 PM, Rohith Surabattula wrote:
On 3/11/2021 8:47 AM, Shyam Prasad N wrote:
Hi Rohith,
The changes look good at a high level.
Just a few points worth checking:
1. In cifs_open(), should be perform deferred close for certain cases
like O_DIRECT? AFAIK, O_DIRECT is just a hint to the filesystem to
perform less data caching. By deferring close, aren't we delaying
flushing dirty pages? @Steve French ?
With O_DIRECT flag, data is not cached locally and will be sent to
server immediately.
2. I see that you're maintaining a new list of files for deferred
closing. Since there could be a large number of such files for a big
share with sufficient I/O, maybe we should think of a structure with
faster lookups (rb trees?).
I know we already have a bunch of linked lists in cifs.ko, and we need
to review perf impact for all those lists. But this one sounds like a
candidate for faster lookups.
Entries will be added into this list only once file is closed and will
remain for acregmax amount interval.
I think you mean once the "file descriptor" is closed, right? But now
that you mention it, caching the attributes sounds a lot like the
NFS close-to-open semantic, which is itself optional with the "nocto"
mount option.
Because some applications use close() to perform flush, there may be
some silent side effects as well. I don't see anything special in the
patch regarding this. Have you considered it?
So, Will this affect performance i.e during lookups ?
3. Minor comment. Maybe change the field name oplock_deferred_close to
oplock_break_received?
Addressed. Attached the patch.
Regards,
Shyam
I wonder why the choice of 5 seconds? It seems to me a more natural
interval on the order of one or more of
- attribute cache timeout
- lease time
- echo_interval
Yes, This sounds good. I modified the patch to defer by attribute
cache timeout interval.
Also, this wording is rather confusing:
When file is closed, SMB2 close request is not sent to server
immediately and is deferred for 5 seconds interval. When file is
reopened by same process for read or write, previous file handle
can be used until oplock is held.
It's not a "previous" filehandle if it's open, and "can be used" is
a rather passive statement. A better wording may be "the filehandle
is reused".
And, "until oplock is held" is similarly awkward. Do you mean "*if*
an oplock is held", or "*provided" an oplock is held"?
When same file is reopened by another client during 5 second
interval, oplock break is sent by server and file is closed immediately
if reference count is zero or else oplock is downgraded.
Only the second part of the sentence is relevant to the patch. Also,
what about lease break?
Modified the patch.
The change to promptly close the handle on oplock or lease break looks
reasonable. The rewording in the patch description is better too.
What happens if the handle is durable or persistent, and the connection
to the server times out? Is the handle recovered, then closed?
Do you mean when the session gets reconnected, the deferred handle
will be recovered and closed?
Yes, because I expect the client to attempt to reclaim its handles upon
reconnection. I don't see any particular code change regarding this.
My question is, if there's a deferred close pending, should it do that,
or should it simply cut to the chase and close any such handle(s)?
Tom.
Regards,
Rohith
On Thu, Mar 11, 2021 at 11:25 PM Tom Talpey <tom@xxxxxxxxxx> wrote:
On 3/11/2021 8:47 AM, Shyam Prasad N wrote:
Hi Rohith,
The changes look good at a high level.
Just a few points worth checking:
1. In cifs_open(), should be perform deferred close for certain cases
like O_DIRECT? AFAIK, O_DIRECT is just a hint to the filesystem to
perform less data caching. By deferring close, aren't we delaying
flushing dirty pages? @Steve French ?
2. I see that you're maintaining a new list of files for deferred
closing. Since there could be a large number of such files for a big
share with sufficient I/O, maybe we should think of a structure with
faster lookups (rb trees?).
I know we already have a bunch of linked lists in cifs.ko, and we need
to review perf impact for all those lists. But this one sounds like a
candidate for faster lookups.
3. Minor comment. Maybe change the field name oplock_deferred_close to
oplock_break_received?
Regards,
Shyam
I wonder why the choice of 5 seconds? It seems to me a more natural
interval on the order of one or more of
- attribute cache timeout
- lease time
- echo_interval
Also, this wording is rather confusing:
When file is closed, SMB2 close request is not sent to server
immediately and is deferred for 5 seconds interval. When file is
reopened by same process for read or write, previous file handle
can be used until oplock is held.
It's not a "previous" filehandle if it's open, and "can be used" is
a rather passive statement. A better wording may be "the filehandle
is reused".
And, "until oplock is held" is similarly awkward. Do you mean "*if*
an oplock is held", or "*provided" an oplock is held"?
When same file is reopened by another client during 5 second
interval, oplock break is sent by server and file is closed immediately
if reference count is zero or else oplock is downgraded.
Only the second part of the sentence is relevant to the patch. Also,
what about lease break?
What happens if the handle is durable or persistent, and the connection
to the server times out? Is the handle recovered, then closed?
Tom.
On Tue, Mar 9, 2021 at 2:41 PM Rohith Surabattula
<rohiths.msft@xxxxxxxxx> wrote:
Hi All,
Please find the attached patch which will defer the close to server.
So, performance can be improved.
i.e When file is open, write, close, open, read, close....
As close is deferred and oplock is held, cache will not be invalidated
and same handle can be used for second open.
Please review the changes and let me know your thoughts.
Regards,
Rohith
