[Top] [All Lists]

Re: [PATCH] xfs: reinstate the iolock in xfs_readdir

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: reinstate the iolock in xfs_readdir
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 4 Dec 2013 05:08:48 -0800
Cc: Ben Myers <bpm@xxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131204001030.GD10988@dastard>
References: <20131203212951.GP1935@xxxxxxx> <20131203225520.GZ10988@dastard> <20131203231415.GS1935@xxxxxxx> <20131204001030.GD10988@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
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
 - 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

<Prev in Thread] Current Thread [Next in Thread>