On Wed, 15 Feb 2012 22:09:53 +0400 Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote: > 2012/2/13 Steve French <smfrench@xxxxxxxxx>: > > On Mon, Feb 13, 2012 at 7:35 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote: > >> On Mon, 13 Feb 2012 12:56:40 +0530 > >> Suresh Jayaraman <sjayaraman@xxxxxxxx> wrote: > >> > >>> On 02/10/2012 11:52 PM, Jeff Layton wrote: > >>> > On Thu, 9 Feb 2012 21:08:12 +0300 > >>> > Pavel Shilovsky <piastry@xxxxxxxxxxx> 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@xxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
