xfs
[Top] [All Lists]

Re: Re: [PATCH 0/8] Set bi_rw when alloc bio before call bio_add_page.

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: Re: [PATCH 0/8] Set bi_rw when alloc bio before call bio_add_page.
From: Muthu Kumar <muthu.lkml@xxxxxxxxx>
Date: Fri, 10 Aug 2012 08:23:41 -0700
Cc: majianpeng <majianpeng@xxxxxxxxx>, Neil Brown <neilb@xxxxxxx>, axboe <axboe@xxxxxxxxx>, "konrad.wilk" <konrad.wilk@xxxxxxxxxx>, "chris.mason" <chris.mason@xxxxxxxxxxxx>, viro <viro@xxxxxxxxxxxxxxxxxx>, tytso <tytso@xxxxxxx>, "adilger.kernel" <adilger.kernel@xxxxxxxxx>, shaggy <shaggy@xxxxxxxxxx>, mfasheh <mfasheh@xxxxxxxx>, jlbec <jlbec@xxxxxxxxxxxx>, bpm <bpm@xxxxxxx>, elder <elder@xxxxxxxxxx>, jfs-discussion <jfs-discussion@xxxxxxxxxxxxxxxxxxxxx>, linux-kernel <linux-kernel@xxxxxxxxxxxxxxx>, xfs <xfs@xxxxxxxxxxx>, linux-btrfs <linux-btrfs@xxxxxxxxxxxxxxx>, linux-ext4 <linux-ext4@xxxxxxxxxxxxxxx>, linux-raid <linux-raid@xxxxxxxxxxxxxxx>, linux-fsdevel <linux-fsdevel@xxxxxxxxxxxxxxx>
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=aVz8sS5eqRcki25pP9fm3D5v3H9MWu2YtPK+WkM4ksA=; b=jMrcl3kApL60ZprPc9kfIdjXN0+ye1eW4Zs6/zFvT0x5owD9lmsEfBRzFnbkK/1ETf 65DgX3Jbaqjg/zW0hZwfwdZPf9LTeadl9VpeX88FwYSwbSnemuec2rDQ5ach0hIQJPz7 lp+Kih6rk1R1fuaa8Cp8BEPwiAgsxTf5NnsOU9F0g2H/aZSRF8S157AeBVITX5p1KWOw ZFdSaVx/7qhefRQyDdOwXZzuqL4hhkw7+KwIIGGAOeuj0X6fJwdXXGiarIj8UnYtUbXU Its4Nkkmbie9S6kByrhRgPmI8UZECxRUDnz36KL2qx6LLJNp8UxRSfnj6qad/D2T8Ugm DMdw==
In-reply-to: <20120731011457.GO2877@dastard>
References: <201207301514247032532@xxxxxxxxx> <20120730214213.GF2877@dastard> <201207310855556258267@xxxxxxxxx> <20120731011457.GO2877@dastard>
Hi,

On Mon, Jul 30, 2012 at 6:14 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> On Tue, Jul 31, 2012 at 08:55:59AM +0800, majianpeng wrote:
>> On 2012-07-31 05:42 Dave Chinner <david@xxxxxxxxxxxxx> Wrote:
>> >On Mon, Jul 30, 2012 at 03:14:28PM +0800, majianpeng wrote:
>> >> When exec bio_alloc, the bi_rw is zero.But after calling bio_add_page,
>> >> it will use bi_rw.
>> >> Fox example, in functiion __bio_add_page,it will call merge_bvec_fn().
>> >> The merge_bvec_fn of raid456 will use the bi_rw to judge the merge.
>> >> >> if ((bvm->bi_rw & 1) == WRITE)
>> >> >> return biovec->bv_len; /* always allow writes to be mergeable */
>> >
>> >So if bio_add_page() requires bi_rw to be set, then shouldn't it be
>> >set up for every caller? I noticed there are about 50 call sites for
>> >bio_add_page(), and you've only touched about 10 of them. Indeed, I
>> >notice that the RAID0/1 code uses bio_add_page, and as that can be
>> >stacked on top of RAID456, it also needs to set bi_rw correctly.
>> >As a result, your patch set is nowhere near complete, not does it
>> >document that bio_add_page requires that bi_rw be set before calling
>> >(which is the new API requirement, AFAICT).
>> There are many place call bio_add_page and I send some of those. Because my abilty, so I only send
>> some patchs which i understand clearly.
>
> Sure, but my point is that there is no point changing only a few and
> ignoring the great majority of callers. Either fix them all, fix it
> some other way (e.g. API change), or remove the code from the RAID5
> function that requires it.
>


A while back, we tried to address this by changing the alloc functions to take rw argument and set it (as per Jens suggestion). I guess the patch did not make it in. Please check:

https://lkml.org/lkml/2011/7/11/275

And the follow ups. If needed, I can dust up that patch and resend it.


>> In __bio_add_page:
>> >>if (q->merge_bvec_fn) {
>> >>                            struct bvec_merge_data bvm = {
>> >>                                    /* prev_bvec is already charged in
>> >>                                       bi_size, discharge it in order to
>> >>                                       simulate merging updated prev_bvec
>> >>                                       as new bvec. */
>> >>                                    .bi_bdev = bio->bi_bdev,
>> >>                                    .bi_sector = bio->bi_sector,
>> >>                                    .bi_size = bio->bi_size - prev_bv_len,
>> >>                                    .bi_rw = bio->bi_rw,
>> >>                            };
>> it used bio->bi_rw.
>> Before raid5_mergeable_bvec appearing, in kernel 'merge_bvec_fn' did not use bio->bi_rw.
>

<snip>


> It's entirely possible that when bi_rw was added to struct
> bvec_merge_data, the person who added it was mistaken that bi_rw was
> set at this point in time when in fact it never has been. Hence it's
> presence and reliance on it would be a bug.
>
> That's what I'm asking - is this actually beneificial, or should it
> simply be removed from struct bvec_merge_data? Data is needed to
> answer that question....


There are cases where we found it really beneficial to know the rw field to decide if the can be really merged or not.

Regards,
Muthu


>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

<Prev in Thread] Current Thread [Next in Thread>