Re: cifs: Deferred close for files

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

Sure Steve.

On Mon, Apr 12, 2021 at 12:19 AM Steve French <smfrench@gmail.com> wrote:
>
> Could you rerun test 422 to Windows? It just failed with:
>
>     -Space used before and after writeback is equal
>     +Files /mnt/scratch/422.before and /mnt/scratch/422.after differ
>
> and tests 023 and 024 to Samba and 024 to Windows which failed with:
>
>     -samedir  tree/none -> none/tree.
>     +samedir  tree/none -> Permission denied
>
> and git tests 0000, 0002, 0003 and 0022 to Windows
>
> See http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/557
>
> It may be unrelated but worked without the patch the previous day:
>
> http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/556
>
> On Sun, Apr 11, 2021 at 7:19 AM Rohith Surabattula
> <rohiths.msft@gmail.com> wrote:
> >
> > Hi All,
> >
> > Have updated the patch with some optimizations.
> >
> > Patch V3:
> > Moved the deferred file list and lock from TCON to inode structures.
> >
> > Regards,
> > Rohith
> >
> > On Wed, Apr 7, 2021 at 8:27 PM Rohith Surabattula
> > <rohiths.msft@gmail.com> wrote:
> > >
> > > Hi All,
> > >
> > > Have updated the patch.
> > >
> > > patch v2:
> > > 1)  Close the file immediately during unlink of file.
> > > 2)  Update the reference count properly when deferred work is already scheduled.
> > >
> > > Regards,
> > > Rohith
> > >
> > > On Thu, Mar 25, 2021 at 8:12 AM Rohith Surabattula
> > > <rohiths.msft@gmail.com> 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?
> > > > >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
> > > > > >>>
> > > > > >>>
> > > > > >>>
>
>
>
> --
> Thanks,
>
> Steve