Hi all,
Today's linux-next merge of the xfs tree got a conflict in
fs/xfs/linux-2.6/xfs_buf.c between commit 7eaceaccab5f ("block: remove
per-queue plugging") from Linus' tree and commit 0e6e847ffe37 ("xfs: stop
using the page cache to back the buffer cache") from the xfs tree.
I assume that these changes (on both sides) were discussed somewhere, but
maybe not clearly enough?
I have no idea how to fix this, so I tried to just use the xfs tree
version for today. That failed like this:
fs/xfs/linux-2.6/xfs_buf.c: In function 'xfs_buf_lock':
fs/xfs/linux-2.6/xfs_buf.c:923: error: implicit declaration of function 'blk_run_backing_dev'
So I used the xfs tree from next-20110325 for today.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
On Mon, Mar 28, 2011 at 12:21:48PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the xfs tree got a conflict in
> fs/xfs/linux-2.6/xfs_buf.c between commit 7eaceaccab5f ("block: remove
> per-queue plugging") from Linus' tree and commit 0e6e847ffe37 ("xfs: stop
> using the page cache to back the buffer cache") from the xfs tree.
>
> I assume that these changes (on both sides) were discussed somewhere, but
> maybe not clearly enough?
>
> I have no idea how to fix this, so I tried to just use the xfs tree
> version for today. That failed like this:
>
> fs/xfs/linux-2.6/xfs_buf.c: In function 'xfs_buf_lock':
> fs/xfs/linux-2.6/xfs_buf.c:923: error: implicit declaration of function 'blk_run_backing_dev'
>
> So I used the xfs tree from next-20110325 for today.
What XFS does is to replace blk_run_address_space, which was a wrapper
around blk_run_backing_dev with a direct call to blk_run_backing_dev,
as there change means we don't have the address_space around anymore.
Jens' tree removes both these functions, and introduces blk_flush_plug
as a sort-of replacement. Sticking to the variant from Jens' tree / mainline
with blk_flush_plug is the correct thing here for this case.
Where there more conflicts than just this?
On Mon, Mar 28, 2011 at 12:47:53PM +0200, Christoph Hellwig wrote:
> What XFS does is to replace blk_run_address_space, which was a wrapper
> around blk_run_backing_dev with a direct call to blk_run_backing_dev,
> as there change means we don't have the address_space around anymore.
>
> Jens' tree removes both these functions, and introduces blk_flush_plug
> as a sort-of replacement. Sticking to the variant from Jens' tree / mainline
> with blk_flush_plug is the correct thing here for this case.
>
> Where there more conflicts than just this?
Actually I think we can remove some calls alltogether: the on-stack
plugging already flushes the plug queue when context switching,
which we'll always do in xfs_buf_wait_unpin, and if we get the lock
without blocking in xfs_buf_lock we don't need to unplug either.
Anyway, that's something to take care off in the XFS tree once it's
merged after the next pull for Linus, no need to keep a fixup that
complicated in linux-next.
On 2011-03-28 12:53, Christoph Hellwig wrote:
> On Mon, Mar 28, 2011 at 12:47:53PM +0200, Christoph Hellwig wrote:
>> What XFS does is to replace blk_run_address_space, which was a wrapper
>> around blk_run_backing_dev with a direct call to blk_run_backing_dev,
>> as there change means we don't have the address_space around anymore.
>>
>> Jens' tree removes both these functions, and introduces blk_flush_plug
>> as a sort-of replacement. Sticking to the variant from Jens' tree / mainline
>> with blk_flush_plug is the correct thing here for this case.
>>
>> Where there more conflicts than just this?
>
> Actually I think we can remove some calls alltogether: the on-stack
> plugging already flushes the plug queue when context switching,
> which we'll always do in xfs_buf_wait_unpin, and if we get the lock
> without blocking in xfs_buf_lock we don't need to unplug either.
Yes, in fact all of the blk_flush_plug() calls in XFS can just go away
now. I tried to keep them for clarity, but they are primarily there
since XFS was the first conversion/testing I did back when it was hacked
up.
--
Jens Axboe
On Mon, Mar 28, 2011 at 12:58:09PM +0200, Jens Axboe wrote:
> Yes, in fact all of the blk_flush_plug() calls in XFS can just go away
> now. I tried to keep them for clarity, but they are primarily there
> since XFS was the first conversion/testing I did back when it was hacked
> up.
It seems like the xfsbufd can go away, too indeed. If we have more
work to do it makes sense not to plug, and if we don't have more
work we are going to sleep.
I think the one in xfs_flush_buftarg actually does make sense to
keep - we really want to flush out all pending I/O before waiting for
it. But I guess for both of these we just want to add an explicit
plug/unlug pair to optimize the I/O dispatch.
Hi Christoph,
On Mon, 28 Mar 2011 12:47:53 +0200 Christoph Hellwig <[email protected]> wrote:
>
> On Mon, Mar 28, 2011 at 12:21:48PM +1100, Stephen Rothwell wrote:
> >
> > Today's linux-next merge of the xfs tree got a conflict in
> > fs/xfs/linux-2.6/xfs_buf.c between commit 7eaceaccab5f ("block: remove
> > per-queue plugging") from Linus' tree and commit 0e6e847ffe37 ("xfs: stop
> > using the page cache to back the buffer cache") from the xfs tree.
> >
> > I assume that these changes (on both sides) were discussed somewhere, but
> > maybe not clearly enough?
> >
> > I have no idea how to fix this, so I tried to just use the xfs tree
> > version for today. That failed like this:
> >
> > fs/xfs/linux-2.6/xfs_buf.c: In function 'xfs_buf_lock':
> > fs/xfs/linux-2.6/xfs_buf.c:923: error: implicit declaration of function 'blk_run_backing_dev'
> >
> > So I used the xfs tree from next-20110325 for today.
>
> What XFS does is to replace blk_run_address_space, which was a wrapper
> around blk_run_backing_dev with a direct call to blk_run_backing_dev,
> as there change means we don't have the address_space around anymore.
>
> Jens' tree removes both these functions, and introduces blk_flush_plug
> as a sort-of replacement. Sticking to the variant from Jens' tree / mainline
> with blk_flush_plug is the correct thing here for this case.
So does that mean that the whole xfs tree commit can be removed/ignored?
That is a bit of a pain to do in a merge (especially considering that
there is another commit in the xfs tree that changes that file. If the
whole commit is no longer needed, maybe it could be dropped from the xfs
tree or reverted.
> Where there more conflicts than just this?
No, the only conflicts (I am pretty sure) were on the lines with the
changed calls. There was, of course, quite a few other changes in that
commit in the xfs tree.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
On Tue, Mar 29, 2011 at 12:37:25AM +1100, Stephen Rothwell wrote:
> So does that mean that the whole xfs tree commit can be removed/ignored?
> That is a bit of a pain to do in a merge (especially considering that
> there is another commit in the xfs tree that changes that file. If the
> whole commit is no longer needed, maybe it could be dropped from the xfs
> tree or reverted.
No, that change is just a small part of the commit.
On Mon, 2011-03-28 at 12:58 +0200, Jens Axboe wrote:
> On 2011-03-28 12:53, Christoph Hellwig wrote:
> > On Mon, Mar 28, 2011 at 12:47:53PM +0200, Christoph Hellwig wrote:
> >> What XFS does is to replace blk_run_address_space, which was a wrapper
> >> around blk_run_backing_dev with a direct call to blk_run_backing_dev,
> >> as there change means we don't have the address_space around anymore.
> >>
> >> Jens' tree removes both these functions, and introduces blk_flush_plug
> >> as a sort-of replacement. Sticking to the variant from Jens' tree / mainline
> >> with blk_flush_plug is the correct thing here for this case.
> >>
> >> Where there more conflicts than just this?
> >
> > Actually I think we can remove some calls alltogether: the on-stack
> > plugging already flushes the plug queue when context switching,
> > which we'll always do in xfs_buf_wait_unpin, and if we get the lock
> > without blocking in xfs_buf_lock we don't need to unplug either.
>
> Yes, in fact all of the blk_flush_plug() calls in XFS can just go away
> now. I tried to keep them for clarity, but they are primarily there
> since XFS was the first conversion/testing I did back when it was hacked
> up.
>
I sent a fix to Linus but he must not have pulled it in
time for the March 29 build. He has pulled it now though.
Ought to be fixed tomorrow.
(To be clear, I just did the simple conflict resolution,
I didn't remove the calls--that'll wait.)
-Alex
FYI, the blk_flush_plug call in ufs_truncate also seem to be almost
certainly incorrect as it's followed by a yield.
That only leaves the RAID code as remaining modular users of it, and
I suspect it really is something that shouldn't need to be exported.
On 2011-03-29 22:08, Christoph Hellwig wrote:
> FYI, the blk_flush_plug call in ufs_truncate also seem to be almost
> certainly incorrect as it's followed by a yield.
>
> That only leaves the RAID code as remaining modular users of it, and
> I suspect it really is something that shouldn't need to be exported.
Agree, once things have settled down we can reap the remaining and keep
it internal.
--
Jens Axboe
On Wed, Mar 30, 2011 at 01:43:55PM +0200, Jens Axboe wrote:
> On 2011-03-29 22:08, Christoph Hellwig wrote:
> > FYI, the blk_flush_plug call in ufs_truncate also seem to be almost
> > certainly incorrect as it's followed by a yield.
> >
> > That only leaves the RAID code as remaining modular users of it, and
> > I suspect it really is something that shouldn't need to be exported.
>
> Agree, once things have settled down we can reap the remaining and keep
> it internal.
In fact I think the blk_flush_plug calls in the raid1/raid10 code can
simply be removed without a replacement. They are in wait_even_loops
which schedule before/after the call so we do get the implicit context
switch plugs there. Even more they are generally called from the
make_request handler, which for MD is in the loop around
__generic_make_request and thus can't actually unplug in a sensible
way.