Re: cifs: Deferred close for files

[Tom Talpey <tom@talpey.com>]

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@gmail.com> 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
> 
> 
>