From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: Ben Myers <bpm@sgi.com>, xfs@oss.sgi.com
Subject: Re: [PATCH] xfs: reinstate the iolock in xfs_readdir
Date: Wed, 4 Dec 2013 05:08:48 -0800 [thread overview]
Message-ID: <20131204130848.GA22926@infradead.org> (raw)
In-Reply-To: <20131204001030.GD10988@dastard>
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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
next prev parent reply other threads:[~2013-12-04 13:08 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-03 21:29 [PATCH] xfs: reinstate the iolock in xfs_readdir Ben Myers
2013-12-03 22:55 ` Dave Chinner
2013-12-03 23:14 ` Ben Myers
2013-12-04 0:10 ` Dave Chinner
2013-12-04 13:08 ` Christoph Hellwig [this message]
2013-12-05 0:12 ` Ben Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131204130848.GA22926@infradead.org \
--to=hch@infradead.org \
--cc=bpm@sgi.com \
--cc=david@fromorbit.com \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox