[linux-cifs-client] SMB_COM_CLOSE and LastWriteTime

[Jeff Layton]

On Fri, 25 Apr 2008 05:27:23 +0200
Guenter Kukkukk <linux at kukkukk.com> wrote:

> Am Donnerstag, 24. April 2008 schrieb Jeff Layton:
> > On Thu, 24 Apr 2008 17:26:58 +0200
> > Guenter Kukkukk <linux at kukkukk.com> wrote:
> > 
> > > Am Donnerstag, 24. April 2008 schrieb Jeff Layton:
> > > > On Thu, 24 Apr 2008 16:13:46 +0200
> > > > Guenter Kukkukk <linux at kukkukk.com> wrote:
> > > > 
> > > > > Am Donnerstag, 24. April 2008 schrieb Guenter Kukkukk:
> > > > > 
> > > > > > An additional note:
> > > > > > i get the following, when i use
> > > > > > cp -p foo /mnt/linux/cifs
> > > > > > cp: preserving permissions for `/mnt/linux/cifs/settime.c': Input/output error
> > > > > > I must specify the "noacl" mount option to avoid this.
> > > > > > Happens when mounting recent 3.0.x or 3.2 samba server.
> > > > > 
> > > > > Ouch - should read  
> > > > > 
> > > > > cp -p settime.c /mnt/linux/cifs
> > > > > cp: preserving permissions for `/mnt/linux/cifs/settime.c': Input/output error
> > > > > 
> > > > > We can leave this one alone atm - could also be a wrong server setup.
> > > > > Cheers, Günter
> > > > 
> > > > 
> > > > Agreed. This doesn't appear to be directly related to the problem with
> > > > setting times.
> > > > 
> > > > I tested out the patch that you sent on March 13 and it seems to fix
> > > > the problem of setting times against win2k3. My only concern is that it
> > > > looks like it rips out some of the code that did an implicit
> > > > open/setattr/close of a file when it's not already open. Some comments
> > > > seem to indicate that older servers needed the file open in order to
> > > > properly set the time. Do we know that this won't break against older
> > > > servers?
> > > > 
> > > > Thanks,
> > > 
> > > I already worked on this area of code about 2 years ago, when Steve and
> > > me tried to add some legacy functionality for os/2 (and win9x/me).
> > > Most of those additions regarding setpathinfo, setfileinfo and also the
> > > corresponding query functions did not really make it into the code that days...
> > > The biggest problem was, that the legacy servers need different info levels
> > > and so also the passed structures like FILE_BASIC_INFO do not match.
> > > 
> > > Not counting legacy stuff, I remember that the special handling for 
> > > CIFS_SES_NT4 made some sense those days, but in my recent more comprehensive
> > > tests i did not "hit any reason for that" anymore.
> > > My recent patch also works with winnt.
> > > Probably Steve can comment on this.
> > > 
> > > I already did some recent work to add more legacy functionality. 
> > > Talked to Steve at sambaxp whether we should place all/most of that
> > > new functions into a separate "legacy" source file. He agreed on this.
> > > One additional needed time conversion function could still be placed into
> > > netmisc.c. But more on that later ...
> > > 
> > 
> > I'm afraid I don't have a working NT4 image, but I did test this patch
> > against Win98se. cp -p always failed to set the mtime for me on that
> > host before. If I understand what wireshark is telling me,
> > Win98se doesn't support SET_PATH_INFO calls. It does support
> > SET_FILE_INFO calls, however, so this patch fixes mtime setting when
> > doing a cp -p to a win98se share (since the file is open we end up
> > using SET_FILE_INFO here).
> > 
> > I suspect though, that we'll probably still need the implicit
> > open/set_file_info/close to make win98se and other early vintage
> > servers work correctly (if it's deemed worthwhile to fix them).
> > 
> > If we care about fixing this universally, we'll some fairly
> > complex logic:
> > 
> > 1) if file is open use SET_FILE_INFO
> 
> Due to the (strange) troubles with windows here, i agree that this
> one should be checked _first_ - unfortunately with some costs!
> Is "find_writable_file(cifsInode);" the _only_ way to check for
> the file open case?
> 

There is is_inode_writable(), which seems to be lighter weight and
doesn't do the implicit reopen. That might be more suitable here.

> > 2) if file is closed and server supports it, use SET_PATH_INFO
> > 3) if file is close and server doesn't support SET_PATH_INFO, then open
> > the file, use SET_FILE_INFO, and close it again.
> 
> I need to setup a win9x/me system to check this a bit more.
> AFAIR - nearly all (legacy) servers support SET_PATH_INFO, it's
> just a question of the requested info level (which isn't done
> right atm).
> I must admit that i never tested against MSDOS/IBMDOS servers....
> 
> Btw - i had a short look at the smbfs sources. I haven't found 
> that "special" sequence
>   open
>   SET_FILE_INFO
>   close
> So i think, we don't need that at all - it should be enough legacy
> support in cifsfs, when it "closely matches smbfs features" ... 


See smb_proc_settime...

smbfs uses the LastWriteTime in the close call to set it to the current
mtime instead of setting it to 0xffffffff. smb_proc_settime just opens
and closes the file. The close sets the LastWriteTime to the inode
mtime.

Perhaps we could use the same trick in CIFS. After all, 2s time
resolution is probably good enough for legacy servers anyway. I'm not
sure about smbfs here though -- looks like the file will be closed
after return from that piece of the code. That may be OK for smbfs, but
I'm not sure about CIFS.

> > 
> > I think that even this is probably too simple, and we might need to
> > special case some stuff, but I suspect this will get most cases...
> > 
> We need to check "the special cases" inside the current code - but
> should put those additional functions into separate (legacy) source
> file(s).
> Most of the legacy functionality is already implemented - and the
> currently known missing ones will NOT blow up the cifs memory footstep
> heavily.
> 
> So i think, we should do it step by step:
>   - add my patch to cifs_setattr()
>   - add a similar change to cifs_unlink()
> to fix winnt, winxp, w2k3 ... 
> Lateron we can add more legacy support in another step.
> 

That seems reasonable to me. It would be nice to not clutter up the
code with special cases for legacy servers.

-- 
Jeff Layton <jlayton at redhat.com>