On Mon, Sep 9, 2013 at 6:56 AM, Jeff Layton <[email protected]> wrote:
> On Sun, 8 Sep 2013 13:18:21 -0400
> Jim McDonough <[email protected]> wrote:
>
>> On Sun, Sep 8, 2013 at 10:32 AM, Jeff Layton <[email protected]> wrote:
>> > On Sat, 7 Sep 2013 14:49:24 -0400
>> > Jim McDonough <[email protected]> wrote:
>> >
>> >> Here's my second try. I hope there aren't missed ops which update the
>> >> inode without having nlink updated. At worst, they should stop
>> >> clobbering valid values, which was definitely happening before.
>> >>
>> >> Signed-off-by: Jim McDonough <[email protected]>
>> >>
>> >> ---
>> >> fs/cifs/cifsglob.h | 1 +
>> >> fs/cifs/inode.c | 21 ++++++++++++++++++++-
>> >> fs/cifs/readdir.c | 3 +++
>> >> 3 files changed, 24 insertions(+), 1 deletion(-)
>> >>
>> >> --- a/fs/cifs/cifsglob.h
>> >> +++ b/fs/cifs/cifsglob.h
>> >> @@ -712,6 +712,7 @@ struct dfs_info3_param {
>> >> #define CIFS_FATTR_DELETE_PENDING 0x2
>> >> #define CIFS_FATTR_NEED_REVAL 0x4
>> >> #define CIFS_FATTR_INO_COLLISION 0x8
>> >> +#define CIFS_FATTR_MISSING_NLINK 0x10
>> >>
>> >> struct cifs_fattr {
>> >> u32 cf_flags;
>> >> --- a/fs/cifs/inode.c
>> >> +++ b/fs/cifs/inode.c
>> >> @@ -118,6 +118,25 @@ cifs_revalidate_cache(struct inode *inod
>> >> cifs_i->invalid_mapping = true;
>> >> }
>> >>
>> >> +/* copy nlink to the inode, unless it wasn't provided. Provide
>> >> + sane values if we don't have an existing one and none was provided */
>> >> +static void
>> >> +cifs_nlink_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
>> >> +{
>> >> + if (fattr->cf_nlink) {
>> >> + inode->i_nlink = fattr->cf_nlink;
>> >> + return;
>> >> + }
>> >> +
>> >
>> > I think it would be best to get rid of the above if block and add an
>> > else block to the end of the following if clause that just sets
>> > inode->i_nlink = fattr->cf_nlink unconditionally.
>> >
>> > The problem with the logic here is that you're ignoring te
>> > (legitimate) case where cf_nlink might be 0 as reported by the server.
>> >
>> Yah, true. I am assuming it won't change to zero after it's been set
>> to something else.
>>
>
> That assumption sounds wrong to me. Consider this case:
Yes, it is a wrong assumption. I wasn't clear enough in trying to agree :-)
>
> You have a file with i_nlink==1 open on the cifs client, so the server
> (maybe samba) also has the file open. A local process on the server
> then unlinks that file and i_nlink then goes to 0 there but because the
> file is open the inode will still stick around. Then the client does
> QueryFileInfo against the open filehandle. The server should report an
> i_nlink of 0 at that point, which would be correct.
>
>> >> + if ((fattr->cf_flags & CIFS_FATTR_MISSING_NLINK) &&
>> >> + (inode->i_state & I_NEW)) {
>> >> + if (fattr->cf_cifsattrs & ATTR_DIRECTORY)
>> >> + inode->i_nlink = 2;
>> >> + else
>> >> + inode->i_nlink = 1;
>> >> + }
>> >> +}
>> >> +
>> >> /* populate an inode with info from a cifs_fattr struct */
>> >> void
>> >> cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
>> >> @@ -132,7 +151,7 @@ cifs_fattr_to_inode(struct inode *inode,
>> >> inode->i_mtime = fattr->cf_mtime;
>> >> inode->i_ctime = fattr->cf_ctime;
>> >> inode->i_rdev = fattr->cf_rdev;
>> >> - inode->i_nlink = fattr->cf_nlink;
>> >> + cifs_nlink_fattr_to_inode(inode, fattr);
>> >> inode->i_uid = fattr->cf_uid;
>> >> inode->i_gid = fattr->cf_gid;
>> >>
>> >> --- a/fs/cifs/readdir.c
>> >> +++ b/fs/cifs/readdir.c
>> >> @@ -130,6 +130,9 @@ cifs_fill_common_info(struct cifs_fattr
>> >> fattr->cf_dtype = DT_REG;
>> >> }
>> >>
>> >> + /* non-unix readdir doesn't provide nlink */
>> >> + fattr->cf_flags &= CIFS_FATTR_MISSING_NLINK
>> >> +
>> >
>> > Shouldn't that be "|=" ?
>> Uh, yeah. I'd already fixed that but posted the wrong one. Sigh.
>> >
>> >> if (fattr->cf_cifsattrs & ATTR_READONLY)
>> >> fattr->cf_mode &= ~S_IWUGO;
>> >>
>> >>
>> >>
>> >>
>> >
>> > The only other thing I see is that the code in cifs_all_info_to_fattr
>> > ought to be changed to use this flag and not try to fake up the
>> > cf_nlink value.
>> Ok, I'll add that as well.
>>
>
>
> --
> Jeff Layton <[email protected]>
--
Jim McDonough
Samba Team
SUSE labs
jmcd at samba dot org
jmcd at themcdonoughs dot org
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
