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
