Skip to contentSkip to conversation

Set bi_rw when alloc bio before call bio_add_page.

PATCH In review majianpeng 4 participants 8 messages Jul 30, 2012 – Aug 10, 2012

Subsystems (4)
VFS Btrfs file system MD RAID OCFS2
lore
Share & tools
lore

majianpeng

author
#0 lore raw

cover Set bi_rw when alloc bio before call bio_add_page.

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 2 lines
Quoted 2 lines

if ((bvm->bi_rw & 1) == WRITE)
return biovec->bv_len; /* always allow writes to be mergeable */

Jianpeng Ma (8):
Evalue bio->bi_rw after calling bio_alloc() and before calling

    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
 block/blk-lib.c                     |    1 +
 drivers/block/xen-blkback/blkback.c |    1 +
 fs/btrfs/check-integrity.c          |    1 +
 fs/direct-io.c                      |    1 +
 fs/ext4/page-io.c                   |    1 +
 fs/jfs/jfs_metapage.c               |    1 +
 fs/ocfs2/cluster/heartbeat.c        |    8 +++++---
 fs/xfs/xfs_aops.c                   |    2 ++
 fs/xfs/xfs_buf.c                    |    1 +
 9 files changed, 14 insertions(+), 3 deletions(-)
Signature · 5 lines
1.7.9.5
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#1 lore raw
Quoted 1 line · majianpeng

When exec bio_alloc, the bi_rw is zero.But after calling bio_add_page,

What about submit_bio? That sets the bi_rw as well?

Quoted 2 lines

it will use bi_rw.
Fox example, in functiion __bio_add_page,it will call merge_bvec_fn().

function.

Quoted 3 lines

The merge_bvec_fn of raid456 will use the bi_rw to judge the merge.

Quoted 2 lines
Quoted 2 lines

if ((bvm->bi_rw & 1) == WRITE)
return biovec->bv_len; /* always allow writes to be mergeable */

So what does that mean? Without this patch what happens?

Quoted 3 lines

Jianpeng Ma (8):

Can you collapse all of this in just one patch?

Quoted 1 line

Evalue bio->bi_rw after calling bio_alloc() and before calling

It is 'evaluate'

Quoted 16 lines
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
Quoted 2 lines
 block/blk-lib.c                     |    1 +
 drivers/block/xen-blkback/blkback.c |    1 +

I am not really sure if that is correct. Does 'submit_bio' not do the job properly?

Quoted 11 lines
 fs/btrfs/check-integrity.c          |    1 +
 fs/direct-io.c                      |    1 +
 fs/ext4/page-io.c                   |    1 +
 fs/jfs/jfs_metapage.c               |    1 +
 fs/ocfs2/cluster/heartbeat.c        |    8 +++++---
 fs/xfs/xfs_aops.c                   |    2 ++
 fs/xfs/xfs_buf.c                    |    1 +
 9 files changed, 14 insertions(+), 3 deletions(-)
Signature · 1 line
1.7.9.5
author
#2 lore raw

On 2012-07-30 23:39 Konrad Rzeszutek Wilk <[email protected]> Wrote:

Quoted 7 lines · majianpeng
Quoted 1 line

When exec bio_alloc, the bi_rw is zero.But after calling bio_add_page,

What about submit_bio? That sets the bi_rw as well?

Quoted 2 lines

it will use bi_rw.
Fox example, in functiion __bio_add_page,it will call merge_bvec_fn().

Quoted 7 lines

function.

Quoted 3 lines

The merge_bvec_fn of raid456 will use the bi_rw to judge the merge.

Quoted 2 lines
Quoted 2 lines

if ((bvm->bi_rw & 1) == WRITE)
return biovec->bv_len; /* always allow writes to be mergeable */

So what does that mean? Without this patch what happens?

Because bio_alloc, the bi_rw is zero default.But in __bio_add_page, it used bi_rw. For example: if merge_bvce_fn used bi_rw, but the bi_rw is wrong(if bi_rw is not zero). So the result may be error.

Quoted 5 lines
Quoted 3 lines

Jianpeng Ma (8):

Can you collapse all of this in just one patch?

Sorry, I think those files are not a subsystem.So i should send separately.

Quoted 20 lines
Quoted 2 lines

Evalue bio->bi_rw after calling bio_alloc() and before calling

It is 'evaluate'

Quoted 16 lines
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
  Evalue bio->bi_rw after calling bio_alloc() and before calling    
    bio_add_page().
Quoted 5 lines
Quoted 2 lines
 block/blk-lib.c                     |    1 +
 drivers/block/xen-blkback/blkback.c |    1 +

I am not really sure if that is correct. Does 'submit_bio' not do the job properly?

Before call submit_bio, it call bio_add_page.

Quoted 10 lines
Quoted 10 lines
 fs/btrfs/check-integrity.c          |    1 +
 fs/direct-io.c                      |    1 +
 fs/ext4/page-io.c                   |    1 +
 fs/jfs/jfs_metapage.c               |    1 +
 fs/ocfs2/cluster/heartbeat.c        |    8 +++++---
 fs/xfs/xfs_aops.c                   |    2 ++
 fs/xfs/xfs_buf.c                    |    1 +
 9 files changed, 14 insertions(+), 3 deletions(-)
Quoted 2 lines
Quoted 2 lines
Signature · 1 line
1.7.9.5

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#3 lore raw
In reply to majianpeng
Quoted 6 lines · majianpeng

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 2 lines
Quoted 2 lines

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).

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....

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]
author
#4 lore raw

On 2012-07-31 05:42 Dave Chinner <[email protected]> Wrote:

Quoted 8 lines · majianpeng
Quoted 6 lines

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 2 lines
Quoted 2 lines

if ((bvm->bi_rw & 1) == WRITE)
return biovec->bv_len; /* always allow writes to be mergeable */

Quoted 8 lines

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 11 lines
Quoted 11 lines
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 8 lines
Quoted 8 lines
	bio = bio_alloc(gfp_mask, 1);
		if (!bio) 
			ret = -ENOMEM;
			
		bio->bi_sector = sector;
		bio->bi_end_io = bio_batch_end_io;
		bio->bi_bdev = bdev;
		bio->bi_private = &bb;

We only add bio->bi_rw = value;
But we shold modify Document for this.

Quoted 8 lines

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.

Quoted 6 lines

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#5 lore raw
Quoted 17 lines · majianpeng

On 2012-07-31 05:42 Dave Chinner <[email protected]> Wrote:

Quoted 8 lines · majianpeng
Quoted 6 lines

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 2 lines
Quoted 2 lines

if ((bvm->bi_rw & 1) == WRITE)
return biovec->bv_len; /* always allow writes to be mergeable */

Quoted 8 lines

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).

Quoted 2 lines

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.

Quoted 12 lines

In __bio_add_page:

Quoted 11 lines
Quoted 11 lines
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,
				};
Quoted 2 lines

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.

Quoted 1 line

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 8 lines
Quoted 7 lines

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.

Signature · 2 lines
Dave Chinner
[email protected]
#6 lore raw

Hi,

Quoted 18 lines · Dave Chinner
Quoted 17 lines · majianpeng

On 2012-07-31 05:42 Dave Chinner <[email protected]> Wrote:

Quoted 16 lines · majianpeng
Quoted 6 lines

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 2 lines
Quoted 2 lines

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).

Quoted 1 line
Quoted 1 line

There are many place call bio_add_page and I send some of those. Because

my abilty, so I only send

Quoted 7 lines
Quoted 1 line

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 5 lines
Quoted 5 lines

In __bio_add_page:

Quoted 4 lines
Quoted 4 lines
if (q->merge_bvec_fn) {
                           struct bvec_merge_data bvm = {
                                   /* prev_bvec is already charged in
                                      bi_size, discharge it in order

to

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

simulate merging updated

prev_bvec

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
                                      as new bvec. */
                                   .bi_bdev = bio->bi_bdev,
                                   .bi_sector = bio->bi_sector,
                                   .bi_size = bio->bi_size -

prev_bv_len,

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
                                   .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.

Quoted 1 line

<snip>

Quoted 8 lines

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

Quoted 12 lines

Cheers,

Dave.
--
Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

------------------------------------------------------------------------------
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/

_______________________________________________
Jfs-discussion mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/jfs-discussion

#7 lore raw
In reply to Dave Chinner

[ Resending in plain text... sorry for the duplicate ]

Hi,

Quoted 21 lines · Dave Chinner
Quoted 19 lines · majianpeng

On 2012-07-31 05:42 Dave Chinner <[email protected]> Wrote:

Quoted 18 lines · majianpeng
Quoted 8 lines

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 2 lines
Quoted 2 lines

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).

Quoted 8 lines
Quoted 3 lines

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.

Quoted 1 line

<snip>

Quoted 9 lines

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

Quoted 12 lines

Cheers,

Dave.
--
Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/