Re: [PATCH v6 04/11] md/r5cache: caching mode of r5cache

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

On Fri, Nov 11 2016, Song Liu wrote:

>  
> +static void
> +r5c_return_dev_pending_writes(struct r5conf *conf, struct r5dev *dev,
> +			      struct bio_list *return_bi)
> +{
> +	struct bio *wbi, *wbi2;
> +
> +	wbi = dev->written;
> +	dev->written = NULL;
> +	while (wbi && wbi->bi_iter.bi_sector <
> +	       dev->sector + STRIPE_SECTORS) {
> +		wbi2 = r5_next_bio(wbi, dev->sector);
> +		if (!raid5_dec_bi_active_stripes(wbi)) {
> +			md_write_end(conf->mddev);
> +			bio_list_add(return_bi, wbi);
> +		}
> +		wbi = wbi2;
> +	}
> +}

This loop (with minor variations) occurs 3 times in raid5.c, and how a
fourth time in raid5-cache.c.  It would be nice if it could be factored
out (maybe as an inline, maybe not) so we only have one copy of the
code.


> +
> +/*
> + * this journal write must contain full parity,
> + * it may also contain some data pages
> + */
> +static void r5c_handle_parity_cached(struct stripe_head *sh)

I don't understand how this function name corresponds to what it does or
when it is called.
It is parts of activating the WRITE_OUT action for a stripe that has (or
may have) been cached to the journal.  None of that is particularly
about "parity".


In generally the patch looks good, but it bothers me that we need to add
tests on R5_InJournal in lots and lots of places.  It makes all those
test sites more complex and so easily misunderstood.
I wonder if there is some way we could add a new flag or something that
would subsumes several of the tests.  So instead of adding a test for InJournal,
we could replace the current test with a test for something new?
Or maybe gather several tests that appear together into an inline.
Or something.

But mostly it looks good.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]