Failure with compound requests and aio enabled

[Christof Schmitt]

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.

Christof