Re: cifs: Deferred close for files

[Tom Talpey <tom@talpey.com>]

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