On Wed, Dec 04, 2013 at 11:10:30AM +1100, Dave Chinner wrote: > The simple fact is that if we ever want to do concurrent directory > read operations, we have to take the ilock in readdir() to ensure we > can serialise correctly against modifications because the i_mutex > can't be used to do that. So, really, I'm not against the fix you > proposed - I'm just trying to understand why it is necessary right > now.... Some comments from the person who authered that old comment that removed the ilock: - as far as I can tell that was not intentional. I'm usually pretty good at recording such things in the changelog if I do it intentionally. - relying on the open function to read in the extent list seems potentially dangerous. We generally try to make sure all the functions using it read it in if needed including the proper locking. If we want to avoid that for some reason like in the writeback path we at least comment it and put asserts in. - like Dave pointed out I think things should just work for mainline modulo maybe the attr by handle ioctl, but it seems by accident. Figuring out what issues Ben sees would be useful. - instead of putting the ilock back at the highest level we might want to push it down to the dir2 data code and only cover the actual critical region where we need it. - xfs_iread_extents really needs an assert to make sure the ilock is held. - xfs_bmapi_read probably should have an assert as well to make sure the ilock is held in some way _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
