On Thu, 10 Jul 2014 01:03:36 -0700 Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > On Tue, Jul 08, 2014 at 02:03:00PM -0400, Jeff Layton wrote: > > ...and replace it with a simple swap call. > > While trying to understand this code I'm failing to grasp the point > of the fi_access counters. Why can't we simply grab a reference to > file everytime we increment fi_access, and simply drop it evertime we > decrement it? > I'm not sure I understand what you're suggesting here. Can you elaborate? AFAIU, the fi_access counters tell us when we're able to zero out the fi_fds slot. fput returns void so we don't have a way to know whether we're putting the last reference to a file or not. Without that, find_*_file become quite problematic -- how would we know whether the pointers they return are still good? > Note that the comment explaining fi_access also seems wrong, as it > suggest contributing 0-4 to the counts, but at best we increment each > one by 1 in a single operation. > Right, we only increment by one for each operation, but open stateids can be upgraded and downgraded. If we do: OPEN for NFS4_SHARE_ACCESS_READ OPEN for NFS4_SHARE_ACCESS_WRITE OPEN for NFS4_SHARE_ACCESS_BOTH ...then we will have incremented the fi_access counts by a total of 4. The big pain in the ass with this code is explained in the comment over bmap_to_share_mode. We have to keep track of what share bits have been previously used in order to comply with the RFC, so you can easily end up with "extra" references. > Also as you touch this area big time: I think fi_fds should be renamed > to fi_fils or similar as we point to file structures and not fds. No objection to the renaming, I'll see if I can work that in on the next respin. -- Jeff Layton <jlayton@xxxxxxxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
