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@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@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 > > >>> > > >>> > > >>>
Attachment:
0001-cifs-Deferred-close-for-files.patch
Description: Binary data
