Re: cifs: Deferred close for files

[Rohith Surabattula <rohiths.msft@gmail.com>]

>>> 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?
>The change to promptly close the handle on oplock or lease break looks
>reasonable. The rewording in the patch description is better too.

Yes, as the handle is closed immediately when oplock/lease breaks,
will there be any
silent side effects still?

>>> 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)?

As the handles are persistent once the session gets reconnected,
applications can reclaim
the handle. So, i believe no need to close handles immediately until
timeout(i.e acregmax interval)

Regards,
Rohith

On Wed, Mar 24, 2021 at 7:50 PM Tom Talpey <tom@talpey.com> wrote:
>
> 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
> >>>
> >>>
> >>>