linux-kernel - Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFw0JG5z-_h7MWMCUK1ZLi_sm+RsEq7=eoQGK2BQCamxXw@mail.gmail.com>
Date:	Fri, 11 Sep 2015 13:02:19 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Chris Mason <clm@...com>, LKML <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Josef Bacik <jbacik@...com>,
	Dave Chinner <david@...morbit.com>, Neil Brown <neilb@...e.de>,
	Jan Kara <jack@...e.cz>, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug()

I hate this fix.

On Fri, Sep 11, 2015 at 12:37 PM, Chris Mason <clm@...com> wrote:
> Linus, this is the plugging problem I mentioned in my btrfs pull.  It
> impacts only MD raid10 and btrfs raid5/6, and I'm not wild about the
> patch. But I wanted to at least send in the basic fix for rc1 so this
> doesn't cause bigger problems for early testers:
>
> Commit d353d7587 added a plug/finish_plug pair to writeback_sb_inodes,
> but writeback_sb_inodes has a horrible secret...it's called with the
> wb->list_lock held.

Quite frankly, just dropping and retaking the lock around the
blk_finish_plug() is just disgusting. The whole "drop and retake lock"
pattern in general is a bad idea, because it can so easily break the
caller (because now the lock no longer covers things over the call.

Yes, in this case we already do something similar in
writeback_single_inode(), so I guess the argument is that it doesn't
make things much worse, and that the caller already cannot depend on
the lock being held. True, but no less disgusting for that. So we
could do this, but in this case I don't think there's any good
_reason_ for doing that disgusting thing.

How about we instead:

 (a) revert that commit d353d7587 as broken (because it clearly is)

 (b) add a big honking comment about the fact that we hold 'list_lock'
in writeback_sb_inodes()

 (c) move the plugging up to wb_writeback() and writeback_inodes_wb()
_outside_ the spinlock.

because that way we not only avoid the ugliness, it should also be
more effective at plugging things since we gather _all_ the writeback
rather than just one superblock.

Let's not paper over a completely broken commit. Let's just admit that
commit d353d7587 was prue and utter shite, and rather than try to fix
up the mistake, make it all better!

Anyway, I will start by reverting that commit, and adding the comment.
I'm more than happy to take the patch that moves the plugging, but
since that one was about performance rather than correctness, I think
it would be good to just re-verify the numbers.

Dave?

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/