[linux-cifs-client] Re: [PATCH] [CIFS] when creating new inodes, use file_mode/dir_mode on mount without unix extensions

[Jeff Layton]

On Wed, 23 Jan 2008 20:27:17 -0500
Jeff Layton <jlayton at redhat.com> wrote:

> On Wed, 23 Jan 2008 12:39:56 -0600
> "Steve French (smfltc)" <smfltc at us.ibm.com> wrote:
> 
> > Jeff Layton wrote:
> > 
> > >On Tue, 22 Jan 2008 18:39:10 -0600
> > >"Steve French (smfltc)" <smfltc at us.ibm.com> wrote:
> > >
> > >  
> > >
> > >>Jeff Layton wrote:
> > >>
> > >>    
> > >>
> > >>>Hi Steve,
> > >>>  I originally sent this patch in late September. We discussed
> > >>> it a
> > >>>bit and you thought it was best to wait until most of the cifsacl
> > >>>patches were in. Now that they are, I'd like to have this patch
> > >>>reconsidered. It just makes it so that file and dir modes are not
> > >>>temporarily overridden when a new inode is created.
> > >>>
> > >>>I've tested it using my modified cthon04 test on a CIFS mount to
> > >>>samba with unix extensions disabled. Everything seems to work as
> > >>>long as you pick sensible file_mode/dir_mode settings. The
> > >>>default file_mode makes it fail, however -- it seems that
> > >>>cthon04 doesn't care for mandatory locking.
> > >>>
> > >>>Thoughts?
> > >>> 
> > >>>
> > >>>      
> > >>>
> > >>This seems risky in a few ways.  With your patch an application
> > >>which wants to restrict the mode of a new file which is creates,
> > >>ie to less than the default mode for the share, would no longer be
> > >>able to.  In the local case you may have a mask that lets you get
> > >>a different mode than you requested, but the result would be less
> > >>than what you ask for (never more than what you asked for on the
> > >>create).
> > >>    
> > >>
> > >
> > >
> > >Right, but it's also possible that the reverse is true -- someone
> > >could specify a restrictive file_mode/dir_mode and assume that when
> > >they create a file that they'll get that mode regardless of what
> > >option is mode is specified in the create call.
> > >  
> > >
> > If we want to treat the mode on the mount like a second mask, that
> > is fine, but we may want
> > to make that configurable.
> > 
> > I would expect a lot of applications (not just ltp tests) would fail
> > to mounts to Windows server if they create files with various
> > different modes and they all end up getting the same mode (due to
> > only using the mode on mount) even taking into account that we can
> > handle removing write mode via +R dos attribute.
> > 
> 
> Most don't care too much about the mode as long as they have
> permissions to read or write as expected. There may be some that will
> fail, but those really can't be expected to work anyway. Eventually
> the inode info will be refreshed, or the fs will get remounted. At
> that point these modes vanish.
> 
> > I have been assuming that the new cifsacl mount option(CIFS acl to
> > mode mapping)  could be too slow for some workloads (and it is
> > still experimental), but it does fix the issue of having the right
> > mode that the application asks for.  Do we need another mount
> > option to indicate whether we want to use the mode specified on the
> > mount or as we do today use the specified mode on lookup of
> > existing inodes but do our best effort on chmod/mkdir/create?
> > 
> 
> Maybe a mount option or other tunable is the best thing for now as a
> transition mechanism. That said, this is really an implementation
> detail, and trying to describe to users when they should use it may be
> a challenge. Once cifsacl is more mature, perhaps we can make it the
> default.
> 
> > >>It also
> > >>seems like we go from a case in which the mode is correct for
> > >>quite a while (but then could revert back to the default for the
> > >>share when the inode is flushed/reloaded), to a case in which the
> > >>mode is never right.   The original idea was that revalidate was
> > >>not supposed to change the mode for a file if the application had
> > >>set a specific mode (create/mkdir/chmod).  There were cases in
> > >>which the inode info would get flushed because e.g. the file were
> > >>not still open and there were memory pressure - and in these
> > >>cases the inode's mode would look different on next lookup, but
> > >>at least it would be correct for a long time (and this is no
> > >>different than what Unix applications deal with everyday - a
> > >>file's mode could change at any time on the server so
> > >>applications have to deal with this already).
> > >>    
> > >>
> > >
> > >It all comes down to what is the behavior of least surprise. IMO,
> > >if someone specifies a file_mode/dir_mode then that's what they
> > >should expect in all cases, not just when the cached inode metadata
> > >times out.
> > >
> > >  
> > >
> > It should not relate to when the metadata times out - if we set the
> > mode locally (but can't set
> > it remotely) we should never overwrite the mode for the same 
> > in-memory-inode from the remote
> > server.  We may have a bug here.
> > 
> 
> Sorry, that's not correct. I meant that eventually the inode will get
> flushed out of the inode cache (either due to memory pressure or for
> other reasons). At that point we'll poll the server for the attrs and
> this mode will go away.
> 
> > >The main problem with this is that we're just talking about the
> > >mode. What about the uid/gid? While the mode is respected here,
> > >the uid/gid of the user creating the file is not. This causes
> > >problems like this:
> > >
> > >  
> > >
> > This is worth investigating/exploring...
> > 
> 
> Trying to preserve the uid/gid info sounds rather scary as well. I
> think we have to be concerned about consistency here. These modes can
> flip to the default at any time and we can't predict when it will
> occur. Non-deterministic behavior is worse than something that
> reliably fails, IMO.
> 
> Eliminating these ephemeral modes is really the best option. vfat, for
> instance, doesn't try to keep modes like this and people use it all
> the time. There may be some apps that break, but presumably those
> users will eventually be able to transition to cifsacl support.
> 


Steve, any further thoughts on this? What would you consider acceptable
as a fix for this bug?

Thanks,
-- 
Jeff Layton <jlayton at redhat.com>