I thought we agreed that (at worst) the logic works for the "noperm" (no client side enforcement, ie multiuser case). On Sat, Oct 29, 2011 at 9:48 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote: > On Sat, 29 Oct 2011 17:03:46 -0500 > Steve French <smfrench@xxxxxxxxx> wrote: > >> On Sat, Oct 29, 2011 at 12:48 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote: >> > On Sat, 29 Oct 2011 00:07:12 -0500 >> > Steve French <smfrench@xxxxxxxxx> wrote: >> > >> >> On Fri, Oct 28, 2011 at 10:44 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote: >> >> > On Fri, 28 Oct 2011 10:27:04 -0500 >> >> > Steve French <smfrench@xxxxxxxxx> wrote: >> >> > >> >> >> On Fri, Oct 28, 2011 at 2:20 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: >> >> >> > On Wed, Oct 19, 2011 at 09:14:37PM -0500, Steve French wrote: >> >> >> >> Doesn't this force the create to happen later - rather than >> >> >> >> at lookup time where it belongs? >> >> >> >> >> >> >> >> if the issue is just noperm ... we should let this through if the user >> >> >> >> has local permissions. Doubling the cost of a file create to Samba >> >> >> >> seems like a bad idea (ie doing aQueryPathInfo AND an NTCreateX >> >> >> >> doubles the roundrip, doubles the load on the server etc.) - the whole >> >> >> >> point of this is to let us do an "atomic" open or create operation >> >> >> >> and not have to split it into multiple requests. In any case why would >> >> >> >> we do the open and then follow it with a create? >> >> >> >> >> >> >> >> Can we fix this to (at least) narrow the performance penalty. >> >> >> >> >> >> >> > >> >> >> > Yes, it does add another back and forth to file create... It's hard/ >> >> >> > impossible to check the permissions first and then decide whether to >> >> >> > pass the O_CREAT flag. Maybe we could add an if statement to check >> >> >> > whether noperm was used and the noperm version could use the create >> >> >> > on lookup. >> >> >> >> >> >> Why is it hard to check local permissions? We have the local mode >> >> >> for the parent. >> >> >> >> >> >> Also note that the "intent" flags and the atomic create is not just >> >> >> about performance, but also making it atomic reduces some of the weird >> >> >> failure cases. >> >> >> >> >> > >> >> > We have the local mode for the parent, but we do not have the ownership >> >> > and mode for the file that has not yet been created. Because of the >> >> > special (ahem) way that cifs handles permissions, it's easily possible >> >> > for the ownership of the file not to match the user doing that create. >> >> > At that point, later operations on the file can easily fail. >> >> > >> >> > This was the primary reason for the multiuser patch series, and why I >> >> > still say that doing permissions checking on the client is a broken >> >> > model. >> >> >> >> We don't have much choice - we have to allow permission >> >> checking on the client when client security context >> >> can't match security context on server - but I would like >> >> to make multiuser the default (especially if we >> >> can figure out a way to integrate winbind-like >> >> upcall for ntlmv2 auth) at least for the future (smb2 etc.) >> >> even if we can't change the default for cifs mounts. >> >> >> > >> > That's fine, but that doesn't solve Dan's immediate problem. We need to >> > either take his patch or something like it for 3.2 (and probably for >> > stable as well) or just rip this crap out altogether. >> >> The benchmark gains are huge for a couple test cases >> (e.g. creating lots of small files) - doesn't make sense to throw it out. >> At worst (I think we can do better) - we only give up on atomic >> open/create for when noperm is not set (we want our users >> to be running noperm or equivalently multiuser). It is especially >> important since the servers we do best at with create turn out >> to be Linux ( and other Unix which Samba runs on (due to posix extensions)). >> > > I don't care at all about performance if the behavior is wrong. We need > to worry about correctness first and only then optimize for > performance. The create on lookup code has been the source of many > regressions and I think it's time to pull the plug on it until someone > wants to take the time to do it right. > > I think it's obvious that the logic in cifs_lookup makes no sense. I'd > prefer to see that ripped out until someone presents a design for this > that does make sense, but if you want to present code that fixes this > more logically then I'd be happy to look at it. > >> If we break create apart as you suggest - we run into >> lookup/getattr/create/open/setattr >> races which are also annoying - we are supposed to send an atomic >> create/open operation >> to minimize strange consequences if the server file changes in between >> (the alternative) >> where getattr/create/open/setattr is done. >> > > We only have to worry about atomicity in the O_EXCL case, and that is > already covered in the earlier check in cifs_lookup. > > -- > Jeff Layton <jlayton@xxxxxxxxx> > -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
