Message-ID: <87bb3444-23b6-837e-afe5-c670c1a2f572@canonical.com>
Date:	Thu, 4 Aug 2016 12:03:41 +0200
From:	Stefan Bader <stefan.bader@...onical.com>
To:	Kent Overstreet <kent.overstreet@...il.com>
Cc:	linux-bcache@...r.kernel.org, dm-devel@...hat.com,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	liuzhengyuang521@...il.com, bcache@...ux.ewheeler.net,
	apw@...onical.com, Stefan Bader <stefan.bader@...onical.com>
Subject: Re: bcache super block corruption with non 4k pages

On 28.07.2016 18:27, Stefan Bader wrote:
> On 28.07.2016 07:55, Kent Overstreet wrote:
>> On Wed, Jul 27, 2016 at 05:16:36PM +0200, Stefan Bader wrote:
>>> So here is another attempt which does half the proposed changes. And before you
>>> point out that it looks still ugly, let me explain some reasons. The goal for me
>>> would be to have something as small as possible to be acceptable as stable change.
>>> And the part about putting a bio on the stack and using submit_bio_wait: I
>>> believe you meant in read_super to replace the __bread. I was thinking about
>>> that but in the end it seemed to make the change unnecessary big. Whether using
>>> __bread or submit_bio_wait, in both cases and without needing to make more
>>> changes on the write side, read_super has to return the in-memory and on-disk
>>> variant of the superblock. So as long as nothing that is related to __bread is
>>> leaked out of read_super, it is much better than what is there now. And I remain
>>> as small as possible for the delta.
>>
>> I like that approach much better. I suppose it's not _strictly_ necessary to rip
>> out the __bread()...
>>
>> Only other comment is that you shouldn't have to pass the buffer to
>> __write_super() - I'd just move the bch_bio_map() call to when the struct
>> cache/cached_dev is being initialized (or rip it out and initialize the bvec by
>> hand) and never touch it after that.
> 
> Hm, doing that would save three simple changes to add a new argument to that
> private functions at the cost of haven the map call twice and a (more)
> complicated calculation of the
>>
>>> So there is one part of the patch which I find hard to do in a better manner but
>>> is a bit ugly: and that is to sanely free the sb_disk_data blob on all error
>>> paths but not on success when it is assigned to either cache or cached_dev.
>>> Could possibly pass the address of the pointer and then clear it inside the
>>> called functions. Not sure that would be much less strange...
>>
>> Yeah that is a hassle - that's why in the 4k superblocks patch in bcache-dev I
>> added that "disk_sb" struct - it owns the buffer and random other crap. You
>> could read that patch for ideas if you want, look at how it transfers ownership
>> of the disk_sb. 
>>
> 
> I had a look but it felt like I could get into too much follow-up changes going
> that path. But I think I got it simpler now. One note about that area: both
> register calls can run into problems but only one actually returns that status.
> And both do not seem to free the allocated structures (cache or cache_dev). It
> is at least not obvious whether this is ever done.
> I working around this by moving the assignment of the buffer page and the
> mapping to a place where an error exit no longer is possible. So the release
> functions will only see a non NULL pointer if things went well (reality required
> to revise that a little bit as one of the register calls that can fail is
> actually doing the write).
> 
> So now there is just one oddness: when I am testing this (unfortunately right
> now I only have a normal 4k case), after calling make-bache with cache and
> backing device, this all looks great and debugging shows the __write_super being
> called. But reading the from userspace will return the old data until I stop the
> bcache device and unregister the cache (which does not show any further writes).
> I cannot decide what I should think here...

So its __bread doing cached operations and after not (ab)using the page from
there, the writes will never cause the cached old result to go away until the
device gets closed. So (hopefully) last apptempt. Also drop __bread and use
submit_bio_wait. This seems to work. On x64 and ppc64le with 64k pages.

So the only pain with this is that upstream seems to have re-organized the
submit_bio functions in the upcoming v4.7-rc2 kernel. So it is without rework
only applicable to older kernels. I was using a 4.4 one, 4.6 in theory should be
ok...

-Stefan


View attachment "0001-bcache-read_super-handle-architectures-with-more-tha.patch" of type "text/x-diff" (8707 bytes)

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists