> On Dec 6, 2021, at 5:52 PM, Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > On Mon, Dec 06, 2021 at 10:30:45PM +0000, Chuck Lever III wrote: >> OK, this is really confusing. >> >> 5142 set_deny(open->op_share_deny, stp); >> 5143 fp->fi_share_deny |= (open->op_share_deny & NFS4_SHARE_DENY_BOTH); >> >> Here set_deny() is treating the contents of open->op_share_deny >> as bit positions, but then upon return NFS4_SHARE_DENY_BOTH >> is used directly as a bit mask. Am I reading this correctly? >> >> But that's not your problem, so I'll let that be. > > This is weird but intentional. > > For most practical purposes, fi_share_deny is all that matters. > > BUT, there is also this language in the spec for OPEN_DOWNGRADE: > > https://datatracker.ietf.org/doc/html/rfc5661#section-18.18.3 > > The bits in share_deny SHOULD equal the union of the share_deny > bits specified for some subset of the OPENs in effect for the > current open-owner on the current file. > > If the above constraints are not respected, the server SHOULD > return the error NFS4ERR_INVAL. > > If you open a file twice, once with DENY_READ, once with DENY_WRITE, > then that is not *quite* the same as opening it once with DENY_BOTH. In > the former case, you're allowed to, for example, downgrade to DENY_READ. > In the latter, you're not. > > So if we want to the server to follow that SHOULD, we need to remember > not only that the union of all the DENYs so far, you also need to > remember the different DENY modes that different OPENs were done with. > > So, we also keep the st_deny_bmap with that information. > > The same goes for allow bits (hence there's also an st_access_bmap). > > It's arguably a lot of extra busy work just for one SHOULD that has no > justification other than just to be persnickety about client > behavior.... Thanks for clarifying! If you are feeling industrious, it would be nice for this to be documented somewhere in the source code.... -- Chuck Lever
