Failure with compound requests and aio enabled

[Christof Schmitt]

On Thu, Sep 21, 2017 at 03:02:39PM -0700, Christof Schmitt wrote:
> On Thu, Sep 21, 2017 at 02:33:34PM -0700, Jeremy Allison wrote:
> > On Thu, Sep 21, 2017 at 12:54:15PM -0700, Christof Schmitt wrote:
> > > 
> > > Thank you for the explanation, i was not aware of this bug. The testcase
> > > was extracting the Linux kernel source code from a tar file on a Mac
> > > client. The problem indeed only occurs on symlinks. The Samba server is
> > > configured with 'aio read size' and 'aio write size' set to 1, as files
> > > can be potentially migrated to offline storage and read/write calls can
> > > block until the data is available.
> > > 
> > > Trying to understand how this breaks the spec: The client sends the
> > > CREATE-WRITE-CLOSE as a compound request. Then the server decides
> > > whether any of those requests are processed asynchronously. If that is
> > > the case, then the server may send an error back to the client:
> > > 
> > > https://msdn.microsoft.com/en-us/library/cc246764.aspx
> > >   If the NextCommand field in the SMB2 header of the request is not
> > >   equal to 0, the server MUST process the received request as a compounded
> > >   series of requests. The server MAY<215> fail requests in a compound
> > >   chain which require asynchronous processing. 
> > > 
> > > https://msdn.microsoft.com/en-us/library/cc246805.aspx#Appendix_A_215
> > >   <215> Section 3.3.5.2.7: In Windows Vista and Windows Server 2008, when
> > >   an operation in a compound request requires asynchronous processing,
> > >   Windows-based servers fail them with STATUS_INTERNAL_ERROR except for
> > >   the following two cases: when a create request in the compound request
> > >   triggers an oplock break, or when the operation is last in the compound
> > >   request.
> > > 
> > > Is that the correct reference to the spec? So i would assume that the
> > > client cannot know beforehand whether the server might try to process a
> > > request asynchronously, so the client has to react to the INTERNAL_ERROR
> > > and retry without compound requests?
> > > 
> > > Interestingly the product behaviour note <215> also implies that newer
> > > versions do not send an error in this case.
> > > 
> > > As this is a bug with existing clients: Would it make sense to have a
> > > similar check for SMB2 requests to not go async when they are part of a
> > > compound chain? This would be similar to the req_is_in_chain() check for
> > > SMB1 requests. I implemented that in the attached patches; any comments
> > > on that?
> > 
> > Nice idea. Can you explain why you need the new bool in_compound_chain ?
> > Can't you use the existing compound_related bool ?
> 
> compound_related is only set from the SMB2_HDR_FLAG_CHAINED flag, which
> is not set in the first request in the chain. The first request has
> SMB2_HDR_NEXT_COMMAND set instead. in_compound_chain would check both to
> mark all requests in the chain from the first to the last.
> 
> Reading the spec again, i am also wondering whether the main check here
> should be only for the SMB2_HDR_NEXT_COMMAND field, as
> SMB2_HDR_FLAG_CHAINED is used to refer to "related" compound requests,
> but here we want to flag any compound request. Using only
> SMB2_HDR_NEXT_COMMAND then raises the question how to identify the last
> request in the compound where that field is 0.

Nevermind, i found a way. schedule_smb2_sendfile_read() already checks
smb2req->in.vector_count to determine whether the request is part of a
compound. I will reuse this check. Updated patch will follow.

Christof