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