[dm-devel] Bcache upstreaming

[Mike Snitzer]

[cc'ing Tejun, adding Kent and linux-bcache back to the cc]

On Fri, Feb 01 2013 at  5:39am -0500,
Lars Ellenberg <lars.ellenberg at linbit.com> wrote:

> On Thu, Jan 31, 2013 at 10:38:11PM -0500, Mike Snitzer wrote:
> > On Thu, Jan 31 2013 at  7:33pm -0500,
> > Kent Overstreet <koverstreet at google.com> wrote:
> > 
> > > On Thu, Jan 31, 2013 at 06:08:00PM -0500, Mike Snitzer wrote:
> > > > On Thu, Jan 31 2013 at  5:25pm -0500,
> > > > Kent Overstreet <kent.overstreet at gmail.com> wrote:
> > > > 
> > > > > On Thu, Jan 31, 2013 at 2:17 PM, Mike Snitzer <snitzer at redhat.com> wrote:
> > > > > > Ah, yeah I had a typo in my script.  When I fixed it I get a BUG (with
> > > > > > your latest bcache code) when I try to mkfs.xfs /dev/bcache0:
> > > > > 
> > > > > Heh, that's the dev branch - you don't want to be running the dev
> > > > > branch, there's a lot of buggy crap in there and it almost definitely
> > > > > corrupts data. Testing branch should be good, though.
> > > > 
> > > > OK, I'll pick up changes from -testing until directed elsewhere.
> > > > 
> > > > BTW, here are couple things I've noticed with bcache:
> > > > 
> > > > - The log messages seem to have an extra newline at the end.
> > > 
> > > How are you seeing that/which log messages? I hadn't noticed that
> > > myself (do they not get printed somehow?)
> > 
> > I'm just seeing extra newlines (empty lines), e.g.:
> > bcache: run_cache_set() invalidating existing data
> > 
> > bcache: bch_cached_dev_attach() Caching dm-4 as bcache1 on set 5c0ba0d0-df36-4684-acc5-45f5b4683788
> > 
> > bcache: register_cache() registered cache device dm-3
> > 
> > EXT4-fs (bcache1): mounted filesystem with ordered data mode. Opts: discard
> > 
> > > > - bcache doesn't appear to be establishing proper holders on the devices
> > > >   it uses for the backing and cache devices.
> > > >   - lsblk doesn't show any associations with bcache devices.
> > > 
> > > How's that created - what function am I looking for?
> > 
> > bd_link_disk_holder and bd_unlink_disk_holder
> 
> Upstream says:
>  * bd_link_disk_holder - create symlinks between holding disk and slave bdev
>  * @bdev: the claimed slave bdev
>  * @disk: the holding disk
>  *
>  * DON'T USE THIS UNLESS YOU'RE ALREADY USING IT.
> ...
> 
>  ?

Well judging by the header for commit 49731baa41df404c2c3f44555869ab387363af43  
("block: restore multiple bd_link_disk_holder() support") it just looks
like Tejun hates the fact that DM and MD are using this interface.  No
alternative is provided; so the "DON'T USE THIS UNLESS YOU'RE ALREADY
USING IT." rings hollow.

Kent was talking about using MD (and though he isn't opposed to DM he
doesn't care to integrate with DM himself).  Either DM or MD would
implicitly enable bcache to use this interface.  But in the near-term I
cannot see why Kent shouldn't be able to use bd_link_disk_holder too.

> > > >   - the fio utility isn't able to get any stats for the bcache device or
> > > >     the devices bcache uses.
> > > 
> > > I'd been meaning to fix that, never got around to figuring out how those
> > > stats are generated. Function/file you can point me to?
> > 
> > I think you'll get the stats for via genhd (add_disk) -- the stats are
> > the 'disk_stats dkstats' member of the genhd's hd_struct.  But as of
> > now you'll notice that /sys/block/bcacheX/stat only ever contains 0s.
> > 
> > part_stat_inc, part_stat_add are the low-level methods for driving the
> > counters up.  But I'm not sure why bcache isn't getting these stats  --
> > disk stats are something we get for free with DM devices so I haven't
> > really had to dig into these mechanics in detail.
> 
> dm core does that for you.
> for other block devices, you'll have to do that yourself.
> 
> My understanding is that you need to do,
> in your make_request, where "disk" is your own struct gendisk*:
>         const int rw = bio_data_dir(bio);
>         int cpu;
>         cpu = part_stat_lock();
>         part_round_stats(cpu, &disk->part0);
>         part_stat_inc(cpu, &disk->part0, ios[rw]);
>         part_stat_add(cpu, &disk->part0, sectors[rw], bio_sectors(bio));
>         part_inc_in_flight(&disk->part0, rw);
>         part_stat_unlock();
> 
> and on completion,
> (you would need to track the "start time" jiffies somewhere):
> 	int rw = bio_data_dir(bio);
>         unsigned long duration = jiffies - start_time;
>         int cpu;
>         cpu = part_stat_lock();
>         part_stat_add(cpu, &disk->part0, ticks[rw], duration);
>         part_round_stats(cpu, &disk->part0);
>         part_dec_in_flight(&disk->part0, rw);
>         part_stat_unlock();

Thanks for clearing that up.