On Thu, Jun 22, 2023 at 12:09:54PM +0200, Jan Kara wrote: > On Wed 21-06-23 13:56:59, Kent Overstreet wrote: > > On Wed, Jun 21, 2023 at 06:23:26PM +0200, Jan Kara wrote: > > > Allocate holder object (cache or cached_dev) before offloading the > > > rest of the startup to async work. This will allow us to open the block > > > block device with proper holder. > > > > This is a pretty big change for this fix, and we'd want to retest the > > error paths - that's hard to do, because the fault injection framework I > > was using for that never made it upstream. > > I agree those are somewhat difficult to test. Although with memory > allocation error injection, we can easily simulate failures in > alloc_holder_object() or say later in bcache_device_init() if that's what > you're after to give at least some testing to the error paths. Admittedly, > I've just tested that registering and unregistering bcache devices works > without giving warnings. Or are you more worried about the "reopen the > block device" logic (and error handling) in the second patch? All error paths need to be tested, yeah. > > What about just exposing a proper API for changing the holder? Wouldn't > > that be simpler? > > It would be doable but frankly I'd prefer to avoid implementing the API for > changing the holder just for bcache. For all I care bcache can also just > generate random cookie (or an id from IDA) and pass it as a holder to > blkdev_get_by_dev(). It would do the job as well and we then don't have to > play games with changing the holder. It would just need to be propagated to > the places doing blkdev_put(). Do you find that better? Well, looking over the code it doesn't seem like it would really simplify the fix, unfortunately. bcachefs has the same issue, but in bcachefs we've already got a handle object where we can allocate and stash a holder - analagous to the bdev_handle you're working on. > I'm now working on a changes that will make blkdev_get_by_dev() return > bdev_handle (which contains bdev pointer but also other stuff we need to > propagate to blkdev_put()) so when that is done, the cookie propagation > will happen automatically. bdev_handle definitely sounds like the right direction. Just changing the holder really seems like it should be easiest to me, and it can get deleted after bdev_handle lands, but maybe there's a new wrinkle I'm not aware of? Anyways, as the error paths get tested I'm ok with this patchset - it's fine if it's done completely by hand (you can just add a goto fail in the appropriate place and make sure nothing explodes). Coli - we should talk about testing at some point. I've been working on test infrastructure; it would be great to get the bcache tests in ktest updated, I've now got a CI we could point at those tests. Christoph - you really gotta start CCing people properly, the patch that broke this didn't even it linux-bcache. No bueno.
