[linux-cifs-client] [PATCH 0/7] [CIFS] clean up cifs_setattr and fix setting of mtimes

[Jeff Layton]

On Sat, 24 May 2008 05:54:45 +0200
Günter Kukkukk <linux at kukkukk.com> wrote:

> Am Freitag, 23. Mai 2008 schrieb Jeff Layton:
> > This patch breaks up the large and unwieldy cifs_setattr function into
> > multiple pieces. In addition to making the function easier to follow and
> > reducing indentation, this should also make it easier to implement a
> > scheme to reduce the amount of times that we retry functions that we
> > know will fail to a particular server. It also fixes the setting of
> > mtimes (LastWriteTime) on open files with Windows servers.
> > 
> > This patch is based on top of the "dynperm" patchset that I've been
> > posting, but which is not yet committed.
> > 
> > Comments and/or suggestions appreciated...
> > 
> > ---
> > 
> > Jeff Layton (7):
> >       [CIFS] turn cifs_setattr into a multiplexor that calls the correct function
> >       [CIFS] move file time and dos attribute setting logic into new function
> >       [CIFS] Rename CIFSSMBSetFileTimes to CIFSSMBSetFileInfo and add PID arg
> >       [CIFS] change CIFSSMBSetTimes to CIFSSMBSetPathInfo
> >       [CIFS] spin off cifs_setattr with unix extensions to its own function
> >       [CIFS] bundle up Unix SET_PATH_INFO args into a struct and change name
> >       [CIFS] break ATTR_SIZE changes out into their own function
> > 
> > 
> >  fs/cifs/cifspdu.h   |    2 
> >  fs/cifs/cifsproto.h |   24 ++
> >  fs/cifs/cifssmb.c   |   43 ++--
> >  fs/cifs/dir.c       |   58 +++--
> >  fs/cifs/file.c      |   19 +-
> >  fs/cifs/inode.c     |  551 ++++++++++++++++++++++++++++++++-------------------
> >  6 files changed, 422 insertions(+), 275 deletions(-)
> > 
> 
> Hi Jeff,
> 
> i fully agree here to "unbundle" the current completely
> overloaded function cifs_setattr() - which has to handle
> too many _very_ different requests.
> 
> As you propose, the very first distinction should be whether
> unix extensions are enabled - or not.
> Then separate for "attr/mode" and "file times" changes.
> Anyway - your approach will help a lot to "implement
> missing legacy functionality"!
> 
> Ouch - your patch set is really large - and i really
> can't tell whether your new stuff is still working...
> 

Yes -- it is large, but if applied in order they should be bisectable.
The sequence of patches in this set actually does matter...

> Jeff, Steve,
> 
> what testcases are you using to check for functionality
> and compatibilty in your own environments?
> Could really help me to do "my own automated tests".
> 

Not much, unfortunately. I use connectathon test suite and things like
fsx and iozone, but they aren't really suited for subtle ferreting out
subtle setattr breakages in cifs.

> Btw - there are similar problems in
> 
> cifs_get_inode_info() ---> CIFSSMBQPathInfo()
> 
> ... completely broken legacy support (and also
> overloaded functions)
> 

Yes -- these problems are all over the place. It's only getting
worse as we add more functionality and handle more types of servers.
IMO, we need to step back a bit and rethink how cifs will evolve in the
future.

Steve and I briefly talked about this at Connectathon. What I'd like to
see is something like the smb_ops struct that smbfs uses. In our case
though, rather than using "const" smb_ops structs, ours would be
declared per-server or per-tcon. At mount time, we can do our best to
set this up, but if the lower functions start returning the right
errors, we can change what function gets called.

My thoughts on it are still pretty fuzzy though, so I haven't actually
started implementing it yet. My thinking is that cifs_setattr could be
set up to do something like:

    pTcon->ops->setattr(...)

...so if that's set up to call cifs_setattr_nounix() and that returns
something like -EOPNOTSUPP, then we'd change the setattr pointer to
point to cifs_setattr_legacy() and try again.

The logic of this would need to be worked out, and also we'd want to
try to start with sane pointers in this struct. That seems like the
difficult part since it's tough to tell ahead of time what functions a
server will support. We also may need to deal with the case where some
functions work against some inodes under a mount, but not others (for
instance if it's possible to cross mountpoints on a server within a
share).

-- 
Jeff Layton <jlayton at redhat.com>