On 2012-07-31 05:42 Dave Chinner [off-list ref] Wrote:
On Mon, Jul 30, 2012 at 03:14:28PM +0800, majianpeng wrote:
quoted
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.
quoted
quoted
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.
In __bio_add_page:
quoted
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.
But i think we shold not suppose bi_rw not meanless.
And I think we not need an new API to do.
Most used bio_alloc and bio_add_page, like this:
quoted
bio = bio_alloc(gfp_mask, 1);
if (!bio)
ret = -ENOMEM;
We only add bio->bi_rw = value;
But we shold modify Document for this.
So, my question is whether the RAID456 code is doing something
valid. That write optimisation is clearly not enabled for a
significant amount of code and filesystems, so the first thing to do
is quantify the benefit of the optimisation. I can't evalute the
merit of this change without data telling me it is worthwhile, and
it's a lot of code to churn for no benefit....
Sorry, we do not think the 'merge_bvec_fn' did not use bi_rw.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
From: Dave Chinner <david@fromorbit.com> Date: 2012-07-31 01:14:57
On Tue, Jul 31, 2012 at 08:55:59AM +0800, majianpeng wrote:
On 2012-07-31 05:42 Dave Chinner [off-list ref] Wrote:
quoted
On Mon, Jul 30, 2012 at 03:14:28PM +0800, majianpeng wrote:
quoted
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.
quoted
quoted
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.
In __bio_add_page:
quoted
quoted
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.
Right, but as things stand right now, the RAID5 code is a no-op
because nobody is setting bio->bi_rw correctly. it is effectively
dead code.
But i think we shold not suppose bi_rw not meanless.
To decide whether we should take it to have meaning, data is
required to show that the RAID5 optimisation it enables is
worthwhile. If the optimisation is not worthwhile, then the correct
thing to do is remove the optimisation in the RAID5 code and remove
the bi_rw field from the struct bvec_merge_data.
quoted
So, my question is whether the RAID456 code is doing something
valid. That write optimisation is clearly not enabled for a
significant amount of code and filesystems, so the first thing to do
is quantify the benefit of the optimisation. I can't evalute the
merit of this change without data telling me it is worthwhile, and
it's a lot of code to churn for no benefit....
Sorry, we do not think the 'merge_bvec_fn' did not use bi_rw.
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....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
Hi,
On Mon, Jul 30, 2012 at 6:14 PM, Dave Chinner [off-list ref] wrote:
On Tue, Jul 31, 2012 at 08:55:59AM +0800, majianpeng wrote:
quoted
On 2012-07-31 05:42 Dave Chinner [off-list ref] Wrote:
quoted
On Mon, Jul 30, 2012 at 03:14:28PM +0800, majianpeng wrote:
quoted
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.
quoted
quoted
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
quoted
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.
quoted
In __bio_add_page:
quoted
quoted
if (q->merge_bvec_fn) {
struct bvec_merge_data bvm = {
/* prev_bvec is already charged in
bi_size, discharge it in order
to
quoted
quoted
quoted
simulate merging updated
prev_bvec
quoted
quoted
quoted
as new bvec. */
.bi_bdev = bio->bi_bdev,
.bi_sector = bio->bi_sector,
.bi_size = bio->bi_size -
prev_bv_len,
quoted
quoted
quoted
.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@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[ Resending in plain text... sorry for the duplicate ]
Hi,
On Mon, Jul 30, 2012 at 6:14 PM, Dave Chinner [off-list ref] wrote:
On Tue, Jul 31, 2012 at 08:55:59AM +0800, majianpeng wrote:
quoted
On 2012-07-31 05:42 Dave Chinner [off-list ref] Wrote:
quoted
On Mon, Jul 30, 2012 at 03:14:28PM +0800, majianpeng wrote:
quoted
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.
quoted
quoted
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.
<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@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/