[Ocfs2-devel] [PATCH 0/8] Set bi_rw when alloc bio before call bio_add_page.

[Ocfs2-devel] [PATCH 0/8] Set bi_rw when alloc bio before call bio_add_page.

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

-- 
1.7.9.5

[Ocfs2-devel] [PATCH 0/8] Set bi_rw when alloc bio before call bio_add_page.

[Konrad Rzeszutek Wilk]

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,

What about submit_bio? That sets the bi_rw as well?
> it will use bi_rw.
> Fox example, in functiion __bio_add_page,it will call merge_bvec_fn().

function.
> 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 what does that mean? Without this patch what happens?

> 
> 
> Jianpeng Ma (8):

Can you collapse all of this in just one patch?

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

It is 'evaluate'
>     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 +

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

>  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(-)
> 
> -- 
> 1.7.9.5

[Ocfs2-devel] [PATCH 0/8] Set bi_rw when alloc bio before call bio_add_page.

[Dave Chinner]

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

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.
-- 
Dave Chinner
david at fromorbit.com

[Ocfs2-devel] [PATCH 0/8] Set bi_rw when alloc bio before call bio_add_page.

[majianpeng]

On 2012-07-30 23:39 Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 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,
>
>What about submit_bio? That sets the bi_rw as well?
>> it will use bi_rw.
>> Fox example, in functiion __bio_add_page,it will call merge_bvec_fn().
>
>function.
>> 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 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.
>> 
>> 
>> 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.
>
>>   Evalue bio->bi_rw after calling bio_alloc() and before calling
>
>It is 'evaluate'
>>     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 +
>
>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.
>
>>  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(-)
>> 
>> -- 
>> 1.7.9.5