Re: [PATCH] cifs: when renaming don't try to unlink negative dentry

On Sat, Apr 18, 2009 at 5:05 AM, Jeff Layton <[email protected]> wrote:
> On Fri, 17 Apr 2009 21:31:50 -0500
> Steve French <[email protected]> wrote:
>
>> On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton <[email protected]> wrote:
>> > On Fri, 17 Apr 2009 16:16:23 -0500
>> > Steve French <[email protected]> wrote:
>> >
>> >> I merged this, adding the CC: stable, but think we need to also
>> >> consistently check for inode == NULL in cifs_unlink (we only check in
>> >> two branches now)
>> >>
>> >> Any objections if I also add the following check:
>> >>
>> >
>> > I think it would be preferable to just check once for inode==NULL in
>> > cifs_unlink near the top and BUG() if it is. Note that vfs_unlink takes
>> > the i_mutex on this before calling the .unlink inode op, so we're
>> > guaranteed that the d_inode won't be NULL from that codepath.
>> >
>> > I think if we get an unlink on a negative dentry then we should
>> > probably consider that a BUG().
>> >
>> > Other .unlink ops also seem to assume that you can't call .unlink with
>> > a negative dentry.
>>
>> I am more worried about internal calls to unlink from cifs slipping
>> through with inode null.  If we check in one branch we should check in
>> the other, or as you suggest move it to the top
>>
>
> Yep, internal callers are my worry too. That's why I think a BUG() is
> appropriate to help catch this when it occurs. Maybe something like
> this patch?

Even if in some strange error path (the kind of cleanup code you added in
various rename paths for example) an internal caller of course has to
have a path (dentry) and should have an inode but I am not convinced
that we should force having a cached inode in all error/cleanup paths -
why is having a local cached copy of the inode fundamentally
required to do unlink (the server state, not the client state is what
is current).   For example, doing this check prevents doing
unlink when there is a negative dentry - but the file still may exist
on the server
(a negative dentry is just a "cached" directory entry - but the file may have
been recently created on the server).  I don't really see a reason why
why we should assume that a negative dentry or dentry without an inode
 (for example, perhaps there are other error cases) should always
prevent us from trying unlink.

I don't mind throwing the cERROR on this case, but it seems odd to require
(in all potential internal unlink uses) an inode if all we are doing
with the inode is
updating cached inode information (if we don't have cached inode information,
we can still send the SMB and do the unlink).

I do agree that today we do have a cached inode in all paths
that call into cifs_unlink in current cifs (presumably that was not
always the case)
(except the negative dentry case which your patch now forbids), but
the existence (or not) of the dentry->d_inode does not really have
anything to do with whether SMB Delete File would work or not.
-- 
Thanks,

Steve