Re: [RFC] atomic open(..., O_CREAT | ...)

[Trond Myklebust <trond.myklebust@fys.uio.no>]

ty den 09.08.2005 Klokka 09:46 (+0200) skreiv Miklos Szeredi:
> > We've already got a patch that does this, and that I'm queueing up for
> > inclusion.
> 
> Cool!
> 
> > http://client.linux-nfs.org/Linux-2.6.x/2.6.12/linux-2.6.12-63-open_file_intents.dif
> 
> Comments:
> 
> >  /*
> > + * Open intents have to release any file pointer that was allocated
> > + * but not used by the VFS.
> > + */
> > +void path_release_open_intent(struct nameidata *nd)
> > +{
> > +	if ((nd->flags & LOOKUP_OPEN) && nd->intent.open.file != NULL) {
> > +		fput(nd->intent.open.file);
> 
> I think you should consider adding this:
> 
> +		if (!IS_ERR(nd->intent.open.file))
> +			fput(nd->intent.open.file);
> 
> so the filesystem can delay returning the error from the open
> operation until the other errors have been sorted out by the lookup
> code.

Intents are meant as optimisations, not replacements for existing
operations. I'm therefore not really comfortable about having them
return errors at all.

> > +		nd->intent.open.file = NULL;
> 
> Why is this NULL assignment needed?  nd will not be used after this.
> 
> > +	}
> > +	path_release(nd);
> > +}
> > +
> > 

It could be dropped. The reason for putting it in is that some parts of
the VFS may restart a path walk operation if it fails (see for instance
the ESTALE handling).

> > As for the "orig flags" thing. What is the point of that?
> 
> dentry_open() needs the original open flags, not the transformed ones
> stored in intent.open.flags.
> 
> The behavior is slightly strange, since filp_open() calculates
> namei_flags (which gets stored in intent.open.flags) so that an
> O_ACCMODE of 3 is transformed into FMODE_READ | FMODE_WRITE.
> 
> But dentry_open() calculates filp->f_mode, so that O_ACCMODE of 3 is
> transformed into zero.
> 
> This means that the (undocumented) access mode of 3 will require
> read-write permission, but will allow neither read() nor write() on
> the opened file.
> 
> If we want to keep this behavior, then the orig_flags field is needed.

Why do we want to keep this behaviour? It is undocumented, it is
non-posix, and it appears to do nothing you cannot do with the existing
access() call.

Are there any applications using it? If so, which ones, and why?

Cheers,
  Trond