[PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Pavel Shilovsky]

Currently we do inc_nlink/drop_nlink for a parent directory for every
mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
because in this case a server doesn't do the same things and returns
the old value on the next QueryInfo request. As the result, we update
our value with the server one and then decrement it on every rmdir
call - go to negative nlink values.

Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
and we still try to decrement it (possible broken servers).

Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/inode.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a5f54b7..9af6ec7 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1355,11 +1355,11 @@ mkdir_retry_old:
 		d_drop(direntry);
 	} else {
 mkdir_get_info:
-		inc_nlink(inode);
-		if (pTcon->unix_ext)
+		if (pTcon->unix_ext) {
+			inc_nlink(inode);
 			rc = cifs_get_inode_info_unix(&newinode, full_path,
 						      inode->i_sb, xid);
-		else
+		} else
 			rc = cifs_get_inode_info(&newinode, full_path, NULL,
 						 inode->i_sb, xid, NULL);
 
@@ -1475,7 +1475,13 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
 	cifs_put_tlink(tlink);
 
 	if (!rc) {
-		drop_nlink(inode);
+		if (pTcon->unix_ext) {
+			if (inode->i_nlink > 2)
+				drop_nlink(inode);
+			else
+				cERROR(1, "%s: unexpected nlink number(%u)",
+				       __func__, inode->i_nlink);
+		}
 		spin_lock(&direntry->d_inode->i_lock);
 		i_size_write(direntry->d_inode, 0);
 		clear_nlink(direntry->d_inode);
-- 
1.7.1

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Shirish Pargaonkar]

On Thu, Feb 9, 2012 at 12:08 PM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> Currently we do inc_nlink/drop_nlink for a parent directory for every
> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
> because in this case a server doesn't do the same things and returns
> the old value on the next QueryInfo request. As the result, we update
> our value with the server one and then decrement it on every rmdir
> call - go to negative nlink values.
>
> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
> and we still try to decrement it (possible broken servers).
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/inode.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index a5f54b7..9af6ec7 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1355,11 +1355,11 @@ mkdir_retry_old:
>                d_drop(direntry);
>        } else {
>  mkdir_get_info:
> -               inc_nlink(inode);
> -               if (pTcon->unix_ext)
> +               if (pTcon->unix_ext) {
> +                       inc_nlink(inode);
>                        rc = cifs_get_inode_info_unix(&newinode, full_path,
>                                                      inode->i_sb, xid);
> -               else
> +               } else
>                        rc = cifs_get_inode_info(&newinode, full_path, NULL,
>                                                 inode->i_sb, xid, NULL);
>
> @@ -1475,7 +1475,13 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>        cifs_put_tlink(tlink);
>
>        if (!rc) {
> -               drop_nlink(inode);
> +               if (pTcon->unix_ext) {
> +                       if (inode->i_nlink > 2)
> +                               drop_nlink(inode);
> +                       else
> +                               cERROR(1, "%s: unexpected nlink number(%u)",
> +                                      __func__, inode->i_nlink);
> +               }
>                spin_lock(&direntry->d_inode->i_lock);
>                i_size_write(direntry->d_inode, 0);
>                clear_nlink(direntry->d_inode);
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Then we need to apply the same logic to set_nlink() calls and
wherever we access i_nlink field also?

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Pavel Shilovsky]

2012/2/10 Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Thu, Feb 9, 2012 at 12:08 PM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>> Currently we do inc_nlink/drop_nlink for a parent directory for every
>> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
>> because in this case a server doesn't do the same things and returns
>> the old value on the next QueryInfo request. As the result, we update
>> our value with the server one and then decrement it on every rmdir
>> call - go to negative nlink values.
>>
>> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
>> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
>> and we still try to decrement it (possible broken servers).
>>
>> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> ---
>>  fs/cifs/inode.c |   14 ++++++++++----
>>  1 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index a5f54b7..9af6ec7 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -1355,11 +1355,11 @@ mkdir_retry_old:
>>                d_drop(direntry);
>>        } else {
>>  mkdir_get_info:
>> -               inc_nlink(inode);
>> -               if (pTcon->unix_ext)
>> +               if (pTcon->unix_ext) {
>> +                       inc_nlink(inode);
>>                        rc = cifs_get_inode_info_unix(&newinode, full_path,
>>                                                      inode->i_sb, xid);
>> -               else
>> +               } else
>>                        rc = cifs_get_inode_info(&newinode, full_path, NULL,
>>                                                 inode->i_sb, xid, NULL);
>>
>> @@ -1475,7 +1475,13 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>>        cifs_put_tlink(tlink);
>>
>>        if (!rc) {
>> -               drop_nlink(inode);
>> +               if (pTcon->unix_ext) {
>> +                       if (inode->i_nlink > 2)
>> +                               drop_nlink(inode);
>> +                       else
>> +                               cERROR(1, "%s: unexpected nlink number(%u)",
>> +                                      __func__, inode->i_nlink);
>> +               }
>>                spin_lock(&direntry->d_inode->i_lock);
>>                i_size_write(direntry->d_inode, 0);
>>                clear_nlink(direntry->d_inode);
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Then we need to apply the same logic to set_nlink() calls and
> wherever we access i_nlink field also?

No, only mkdir/rmdir need this. Other places in cifs with i_nlink
access is either read only or inc_nlink/drop_nlink for
non-directories.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Jeff Layton]

On Thu,  9 Feb 2012 21:08:12 +0300
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> Currently we do inc_nlink/drop_nlink for a parent directory for every
> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
> because in this case a server doesn't do the same things and returns
> the old value on the next QueryInfo request. As the result, we update
> our value with the server one and then decrement it on every rmdir
> call - go to negative nlink values.
> 
> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
> and we still try to decrement it (possible broken servers).
> 

Rather than doing that, I think it would be better not to do the
inc/dec_nlink in either case and instead to set cifsi->time on the
parent to 0 for both cases.

That should force it to have the directory attributes refetched at the
next opportunity. Since we're not doing that now, we're likely missing
out on stuff like directory mtime changes as well.

> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/inode.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index a5f54b7..9af6ec7 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1355,11 +1355,11 @@ mkdir_retry_old:
>  		d_drop(direntry);
>  	} else {
>  mkdir_get_info:
> -		inc_nlink(inode);
> -		if (pTcon->unix_ext)
> +		if (pTcon->unix_ext) {
> +			inc_nlink(inode);
>  			rc = cifs_get_inode_info_unix(&newinode, full_path,
>  						      inode->i_sb, xid);
> -		else
> +		} else
>  			rc = cifs_get_inode_info(&newinode, full_path, NULL,
>  						 inode->i_sb, xid, NULL);
>  
> @@ -1475,7 +1475,13 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>  	cifs_put_tlink(tlink);
>  
>  	if (!rc) {
> -		drop_nlink(inode);
> +		if (pTcon->unix_ext) {
> +			if (inode->i_nlink > 2)
> +				drop_nlink(inode);
> +			else
> +				cERROR(1, "%s: unexpected nlink number(%u)",
> +				       __func__, inode->i_nlink);
> +		}
>  		spin_lock(&direntry->d_inode->i_lock);
>  		i_size_write(direntry->d_inode, 0);
>  		clear_nlink(direntry->d_inode);


-- 
Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Pavel Shilovsky]

2012/2/10 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
> On Thu,  9 Feb 2012 21:08:12 +0300
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> Currently we do inc_nlink/drop_nlink for a parent directory for every
>> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
>> because in this case a server doesn't do the same things and returns
>> the old value on the next QueryInfo request. As the result, we update
>> our value with the server one and then decrement it on every rmdir
>> call - go to negative nlink values.
>>
>> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
>> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
>> and we still try to decrement it (possible broken servers).
>>
>
> Rather than doing that, I think it would be better not to do the
> inc/dec_nlink in either case and instead to set cifsi->time on the
> parent to 0 for both cases.
>
> That should force it to have the directory attributes refetched at the
> next opportunity. Since we're not doing that now, we're likely missing
> out on stuff like directory mtime changes as well.
>

I think this is a good idea, thanks! I'll try it and post a patch if it's ok.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Suresh Jayaraman]

On 02/10/2012 11:52 PM, Jeff Layton wrote:
> On Thu,  9 Feb 2012 21:08:12 +0300
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 
>> Currently we do inc_nlink/drop_nlink for a parent directory for every
>> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
>> because in this case a server doesn't do the same things and returns
>> the old value on the next QueryInfo request. As the result, we update
>> our value with the server one and then decrement it on every rmdir
>> call - go to negative nlink values.
>>
>> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
>> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
>> and we still try to decrement it (possible broken servers).
>>
> 
> Rather than doing that, I think it would be better not to do the
> inc/dec_nlink in either case and instead to set cifsi->time on the
> parent to 0 for both cases.
> 
> That should force it to have the directory attributes refetched at the
> next opportunity. Since we're not doing that now, we're likely missing
> out on stuff like directory mtime changes as well.
> 

Hmm.. don't we have to do both? Keep the nlink value sane and force
refetching attrs. Wondering if we don't update nlink what will happen in
cases

   (a) when mkdir/rmdir is run in a tight loop
   (b) when a dir is moved from one to another within the cifs mount


Thanks
Suresh

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Jeff Layton]

On Mon, 13 Feb 2012 12:56:40 +0530
Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:

> On 02/10/2012 11:52 PM, Jeff Layton wrote:
> > On Thu,  9 Feb 2012 21:08:12 +0300
> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> > 
> >> Currently we do inc_nlink/drop_nlink for a parent directory for every
> >> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
> >> because in this case a server doesn't do the same things and returns
> >> the old value on the next QueryInfo request. As the result, we update
> >> our value with the server one and then decrement it on every rmdir
> >> call - go to negative nlink values.
> >>
> >> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
> >> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
> >> and we still try to decrement it (possible broken servers).
> >>
> > 
> > Rather than doing that, I think it would be better not to do the
> > inc/dec_nlink in either case and instead to set cifsi->time on the
> > parent to 0 for both cases.
> > 
> > That should force it to have the directory attributes refetched at the
> > next opportunity. Since we're not doing that now, we're likely missing
> > out on stuff like directory mtime changes as well.
> > 
> 
> Hmm.. don't we have to do both? Keep the nlink value sane and force
> refetching attrs. Wondering if we don't update nlink what will happen in
> cases
> 
>    (a) when mkdir/rmdir is run in a tight loop
>    (b) when a dir is moved from one to another within the cifs mount
> 

I don't think so -- we either need to fake i_nlink and ignore the value
from the server, or treat the server as authoritative.

Trying to monkey with the nlink value on the client and overwriting it
with the value from the server is always going to be racy. I think the
only time it really matters is if you're using generic_drop_inode,
which we do when CIFS_MOUNT_SERVER_INUM is set.

That said, the values we get out of some servers for i_nlink are
non-sensical. Perhaps we'd be best off to just fake the i_nlink value
across the board. We have had people in the past complain that i_nlink
is always 0 in some cases.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Steve French]

On Mon, Feb 13, 2012 at 7:35 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Mon, 13 Feb 2012 12:56:40 +0530
> Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:
>
>> On 02/10/2012 11:52 PM, Jeff Layton wrote:
>> > On Thu,  9 Feb 2012 21:08:12 +0300
>> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>> >
>> >> Currently we do inc_nlink/drop_nlink for a parent directory for every
>> >> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
>> >> because in this case a server doesn't do the same things and returns
>> >> the old value on the next QueryInfo request. As the result, we update
>> >> our value with the server one and then decrement it on every rmdir
>> >> call - go to negative nlink values.
>> >>
>> >> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
>> >> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
>> >> and we still try to decrement it (possible broken servers).
>> >>
>> >
>> > Rather than doing that, I think it would be better not to do the
>> > inc/dec_nlink in either case and instead to set cifsi->time on the
>> > parent to 0 for both cases.
>> >
>> > That should force it to have the directory attributes refetched at the
>> > next opportunity. Since we're not doing that now, we're likely missing
>> > out on stuff like directory mtime changes as well.
>> >
>>
>> Hmm.. don't we have to do both? Keep the nlink value sane and force
>> refetching attrs. Wondering if we don't update nlink what will happen in
>> cases
>>
>>    (a) when mkdir/rmdir is run in a tight loop
>>    (b) when a dir is moved from one to another within the cifs mount
>>
>
> I don't think so -- we either need to fake i_nlink and ignore the value
> from the server, or treat the server as authoritative.
>
> Trying to monkey with the nlink value on the client and overwriting it
> with the value from the server is always going to be racy. I think the
> only time it really matters is if you're using generic_drop_inode,
> which we do when CIFS_MOUNT_SERVER_INUM is set.
>
> That said, the values we get out of some servers for i_nlink are
> non-sensical. Perhaps we'd be best off to just fake the i_nlink value
> across the board. We have had people in the past complain that i_nlink
> is always 0 in some cases.

For the non-Unix case, you may be right.  We wouldn't want to
add a big performance penalty to do QueryPathInfo more often
for a value which the server may report wrong anyway.


-- 
Thanks,

Steve

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Pavel Shilovsky]

2012/2/13 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Mon, Feb 13, 2012 at 7:35 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> On Mon, 13 Feb 2012 12:56:40 +0530
>> Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:
>>
>>> On 02/10/2012 11:52 PM, Jeff Layton wrote:
>>> > On Thu,  9 Feb 2012 21:08:12 +0300
>>> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>>> >
>>> >> Currently we do inc_nlink/drop_nlink for a parent directory for every
>>> >> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
>>> >> because in this case a server doesn't do the same things and returns
>>> >> the old value on the next QueryInfo request. As the result, we update
>>> >> our value with the server one and then decrement it on every rmdir
>>> >> call - go to negative nlink values.
>>> >>
>>> >> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
>>> >> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
>>> >> and we still try to decrement it (possible broken servers).
>>> >>
>>> >
>>> > Rather than doing that, I think it would be better not to do the
>>> > inc/dec_nlink in either case and instead to set cifsi->time on the
>>> > parent to 0 for both cases.
>>> >
>>> > That should force it to have the directory attributes refetched at the
>>> > next opportunity. Since we're not doing that now, we're likely missing
>>> > out on stuff like directory mtime changes as well.
>>> >
>>>
>>> Hmm.. don't we have to do both? Keep the nlink value sane and force
>>> refetching attrs. Wondering if we don't update nlink what will happen in
>>> cases
>>>
>>>    (a) when mkdir/rmdir is run in a tight loop
>>>    (b) when a dir is moved from one to another within the cifs mount
>>>
>>
>> I don't think so -- we either need to fake i_nlink and ignore the value
>> from the server, or treat the server as authoritative.
>>
>> Trying to monkey with the nlink value on the client and overwriting it
>> with the value from the server is always going to be racy. I think the
>> only time it really matters is if you're using generic_drop_inode,
>> which we do when CIFS_MOUNT_SERVER_INUM is set.
>>
>> That said, the values we get out of some servers for i_nlink are
>> non-sensical. Perhaps we'd be best off to just fake the i_nlink value
>> across the board. We have had people in the past complain that i_nlink
>> is always 0 in some cases.
>
> For the non-Unix case, you may be right.  We wouldn't want to
> add a big performance penalty to do QueryPathInfo more often
> for a value which the server may report wrong anyway.
>
>

So, It seems that ignoring NumberOfLinks value from FILE_ALL_INFO
structure on QueryPathInfo in non-POSIX case is the best we can do. If
nobody objects I will create a patch for this.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix mkdir/rmdir bug for the non-POSIX case

[Jeff Layton]

On Wed, 15 Feb 2012 22:09:53 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> 2012/2/13 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > On Mon, Feb 13, 2012 at 7:35 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >> On Mon, 13 Feb 2012 12:56:40 +0530
> >> Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:
> >>
> >>> On 02/10/2012 11:52 PM, Jeff Layton wrote:
> >>> > On Thu,  9 Feb 2012 21:08:12 +0300
> >>> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >>> >
> >>> >> Currently we do inc_nlink/drop_nlink for a parent directory for every
> >>> >> mkdir and rmdir calls. That's wrong when POSIX extensions are disabled
> >>> >> because in this case a server doesn't do the same things and returns
> >>> >> the old value on the next QueryInfo request. As the result, we update
> >>> >> our value with the server one and then decrement it on every rmdir
> >>> >> call - go to negative nlink values.
> >>> >>
> >>> >> Fix this by doing inc_nlink/drop_nlink for parent directory in mkdir
> >>> >> and rmdir in POSIX case only. Also add cERROR when nlink value <= 2
> >>> >> and we still try to decrement it (possible broken servers).
> >>> >>
> >>> >
> >>> > Rather than doing that, I think it would be better not to do the
> >>> > inc/dec_nlink in either case and instead to set cifsi->time on the
> >>> > parent to 0 for both cases.
> >>> >
> >>> > That should force it to have the directory attributes refetched at the
> >>> > next opportunity. Since we're not doing that now, we're likely missing
> >>> > out on stuff like directory mtime changes as well.
> >>> >
> >>>
> >>> Hmm.. don't we have to do both? Keep the nlink value sane and force
> >>> refetching attrs. Wondering if we don't update nlink what will happen in
> >>> cases
> >>>
> >>>    (a) when mkdir/rmdir is run in a tight loop
> >>>    (b) when a dir is moved from one to another within the cifs mount
> >>>
> >>
> >> I don't think so -- we either need to fake i_nlink and ignore the value
> >> from the server, or treat the server as authoritative.
> >>
> >> Trying to monkey with the nlink value on the client and overwriting it
> >> with the value from the server is always going to be racy. I think the
> >> only time it really matters is if you're using generic_drop_inode,
> >> which we do when CIFS_MOUNT_SERVER_INUM is set.
> >>
> >> That said, the values we get out of some servers for i_nlink are
> >> non-sensical. Perhaps we'd be best off to just fake the i_nlink value
> >> across the board. We have had people in the past complain that i_nlink
> >> is always 0 in some cases.
> >
> > For the non-Unix case, you may be right.  We wouldn't want to
> > add a big performance penalty to do QueryPathInfo more often
> > for a value which the server may report wrong anyway.
> >
> >
> 
> So, It seems that ignoring NumberOfLinks value from FILE_ALL_INFO
> structure on QueryPathInfo in non-POSIX case is the best we can do. If
> nobody objects I will create a patch for this.
> 

I think there are several pieces needed here:

1) get rid of the inc/dec_nlink calls on the parent inode altogether.
Trying to do that on the client is never going to make any sense.
Replace them with a reset of the cifsi->time to 0 to force a reval of
the inode attributes at some point in the future.

2) fake the i_nlink value on directories for the non-posix case. In
other cases, NumberOfLinks is probably something we should use.

3) maybe fix cifs_drop_inode to handle this situation correctly? It
seems like calling generic_drop_inode on it when we're faking i_nlink
would be wrong, but maybe it's ok. I'm not sure.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>