diff mbox

[02/17] block: introduce bio_remove_last_page()

Message ID 1487245547-24384-3-git-send-email-tom.leiming@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ming Lei Feb. 16, 2017, 11:45 a.m. UTC
MD need this helper to remove the last added page, so introduce
it.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 block/bio.c         | 23 +++++++++++++++++++++++
 include/linux/bio.h |  1 +
 2 files changed, 24 insertions(+)

Comments

Johannes Thumshirn Feb. 16, 2017, 12:08 p.m. UTC | #1
On 02/16/2017 12:45 PM, Ming Lei wrote:
> MD need this helper to remove the last added page, so introduce
> it.
> 
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
>  block/bio.c         | 23 +++++++++++++++++++++++
>  include/linux/bio.h |  1 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 5eec5e08417f..0ce7ffcd7939 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>  EXPORT_SYMBOL(bio_add_pc_page);
>  
>  /**
> + *	bio_remove_last_page	-	remove the last added page
> + *	@bio: destination bio
> + *
> + *	Attempt to remove the last added page from the bio_vec maplist.
> + */
> +void bio_remove_last_page(struct bio *bio)
> +{
> +	/*
> +	 * cloned bio must not modify vec list
> +	 */
> +	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
> +		return;
> +
> +	if (bio->bi_vcnt > 0) {

In patch 1 you introduce bio_segments_all() with the log message 'So
that we can replace the direct access to .bi_vcnt.' Here you introduce a
new direct access to it (plus the duplicated WARN_ON_ONCE()).

Maybe use the helper directly here (I admit I haven't gone through the
whole series yet, so I can't see if the change is made later).

Byte,
	Johannes
Ming Lei Feb. 16, 2017, 1:30 p.m. UTC | #2
On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On 02/16/2017 12:45 PM, Ming Lei wrote:
>> MD need this helper to remove the last added page, so introduce
>> it.
>>
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> ---
>>  block/bio.c         | 23 +++++++++++++++++++++++
>>  include/linux/bio.h |  1 +
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 5eec5e08417f..0ce7ffcd7939 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>>  EXPORT_SYMBOL(bio_add_pc_page);
>>
>>  /**
>> + *   bio_remove_last_page    -       remove the last added page
>> + *   @bio: destination bio
>> + *
>> + *   Attempt to remove the last added page from the bio_vec maplist.
>> + */
>> +void bio_remove_last_page(struct bio *bio)
>> +{
>> +     /*
>> +      * cloned bio must not modify vec list
>> +      */
>> +     if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>> +             return;
>> +
>> +     if (bio->bi_vcnt > 0) {
>
> In patch 1 you introduce bio_segments_all() with the log message 'So
> that we can replace the direct access to .bi_vcnt.' Here you introduce a
> new direct access to it (plus the duplicated WARN_ON_ONCE()).
>
> Maybe use the helper directly here (I admit I haven't gone through the
> whole series yet, so I can't see if the change is made later).

Firstly MD does need one helper to remove the last added page, as you
can see there are three such uses in patch3.

Secondly both the two helpers will be changed once multipage bvec
is supported, that means we have to change MD too after multipage bvec
if just using bio_segments_all() to replace .bi_vcnt for removing
the last added page.



Thanks,
Ming Lei
Johannes Thumshirn Feb. 16, 2017, 1:40 p.m. UTC | #3
On 02/16/2017 02:30 PM, Ming Lei wrote:
> On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>> On 02/16/2017 12:45 PM, Ming Lei wrote:
>>> MD need this helper to remove the last added page, so introduce
>>> it.
>>>
>>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>>> ---
>>>  block/bio.c         | 23 +++++++++++++++++++++++
>>>  include/linux/bio.h |  1 +
>>>  2 files changed, 24 insertions(+)
>>>
>>> diff --git a/block/bio.c b/block/bio.c
>>> index 5eec5e08417f..0ce7ffcd7939 100644
>>> --- a/block/bio.c
>>> +++ b/block/bio.c
>>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>>>  EXPORT_SYMBOL(bio_add_pc_page);
>>>
>>>  /**
>>> + *   bio_remove_last_page    -       remove the last added page
>>> + *   @bio: destination bio
>>> + *
>>> + *   Attempt to remove the last added page from the bio_vec maplist.
>>> + */
>>> +void bio_remove_last_page(struct bio *bio)
>>> +{
>>> +     /*
>>> +      * cloned bio must not modify vec list
>>> +      */
>>> +     if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>>> +             return;
>>> +
>>> +     if (bio->bi_vcnt > 0) {
>>
>> In patch 1 you introduce bio_segments_all() with the log message 'So
>> that we can replace the direct access to .bi_vcnt.' Here you introduce a
>> new direct access to it (plus the duplicated WARN_ON_ONCE()).
>>
>> Maybe use the helper directly here (I admit I haven't gone through the
>> whole series yet, so I can't see if the change is made later).
> 
> Firstly MD does need one helper to remove the last added page, as you
> can see there are three such uses in patch3.
> 
> Secondly both the two helpers will be changed once multipage bvec
> is supported, that means we have to change MD too after multipage bvec
> if just using bio_segments_all() to replace .bi_vcnt for removing
> the last added page.

I'm not sure if we're talking past each other here, I assumed you'd do
something like:

void bio_remove_last_page(struct bio *bio)
{
	int vcnt = bio_segments_all(bio);

	if (bio_flagged(bio, BIO_CLONED))
		return;

	if (vcnt > 0) {
		struct bio_vec *bv = &bio->bi_io_vec[vcnt - 1];

		bio->bi_iter.bi_size -= bv->bv_len;
		bio->bi_vcnt--;
	}
}
Ming Lei Feb. 16, 2017, 1:59 p.m. UTC | #4
On Thu, Feb 16, 2017 at 9:40 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On 02/16/2017 02:30 PM, Ming Lei wrote:
>> On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>>> On 02/16/2017 12:45 PM, Ming Lei wrote:
>>>> MD need this helper to remove the last added page, so introduce
>>>> it.
>>>>
>>>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>>>> ---
>>>>  block/bio.c         | 23 +++++++++++++++++++++++
>>>>  include/linux/bio.h |  1 +
>>>>  2 files changed, 24 insertions(+)
>>>>
>>>> diff --git a/block/bio.c b/block/bio.c
>>>> index 5eec5e08417f..0ce7ffcd7939 100644
>>>> --- a/block/bio.c
>>>> +++ b/block/bio.c
>>>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>>>>  EXPORT_SYMBOL(bio_add_pc_page);
>>>>
>>>>  /**
>>>> + *   bio_remove_last_page    -       remove the last added page
>>>> + *   @bio: destination bio
>>>> + *
>>>> + *   Attempt to remove the last added page from the bio_vec maplist.
>>>> + */
>>>> +void bio_remove_last_page(struct bio *bio)
>>>> +{
>>>> +     /*
>>>> +      * cloned bio must not modify vec list
>>>> +      */
>>>> +     if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>>>> +             return;
>>>> +
>>>> +     if (bio->bi_vcnt > 0) {
>>>
>>> In patch 1 you introduce bio_segments_all() with the log message 'So
>>> that we can replace the direct access to .bi_vcnt.' Here you introduce a
>>> new direct access to it (plus the duplicated WARN_ON_ONCE()).
>>>
>>> Maybe use the helper directly here (I admit I haven't gone through the
>>> whole series yet, so I can't see if the change is made later).
>>
>> Firstly MD does need one helper to remove the last added page, as you
>> can see there are three such uses in patch3.
>>
>> Secondly both the two helpers will be changed once multipage bvec
>> is supported, that means we have to change MD too after multipage bvec
>> if just using bio_segments_all() to replace .bi_vcnt for removing
>> the last added page.
>
> I'm not sure if we're talking past each other here, I assumed you'd do
> something like:
>
> void bio_remove_last_page(struct bio *bio)
> {
>         int vcnt = bio_segments_all(bio);
>
>         if (bio_flagged(bio, BIO_CLONED))
>                 return;
>
>         if (vcnt > 0) {
>                 struct bio_vec *bv = &bio->bi_io_vec[vcnt - 1];
>
>                 bio->bi_iter.bi_size -= bv->bv_len;
>                 bio->bi_vcnt--;
>         }
> }

What we are doing is to remove the external direct access to bvec
table in drivers or filesystems because they may misuse that, for
example, drivers often use .bi_vcnt to get the page count in the bio,
but the actual meaning is just bvec's count.

And the implementation in block layer still need to play the table directly,
especially for sake of efficiency, cause we understand the details and won't
misuse that.


Thanks,
Ming Lei
Johannes Thumshirn Feb. 16, 2017, 2:10 p.m. UTC | #5
On 02/16/2017 02:59 PM, Ming Lei wrote:
> On Thu, Feb 16, 2017 at 9:40 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>> On 02/16/2017 02:30 PM, Ming Lei wrote:
>>> On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>>>> On 02/16/2017 12:45 PM, Ming Lei wrote:
>>>>> MD need this helper to remove the last added page, so introduce
>>>>> it.
>>>>>
>>>>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>>>>> ---
>>>>>  block/bio.c         | 23 +++++++++++++++++++++++
>>>>>  include/linux/bio.h |  1 +
>>>>>  2 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/block/bio.c b/block/bio.c
>>>>> index 5eec5e08417f..0ce7ffcd7939 100644
>>>>> --- a/block/bio.c
>>>>> +++ b/block/bio.c
>>>>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>>>>>  EXPORT_SYMBOL(bio_add_pc_page);
>>>>>
>>>>>  /**
>>>>> + *   bio_remove_last_page    -       remove the last added page
>>>>> + *   @bio: destination bio
>>>>> + *
>>>>> + *   Attempt to remove the last added page from the bio_vec maplist.
>>>>> + */
>>>>> +void bio_remove_last_page(struct bio *bio)
>>>>> +{
>>>>> +     /*
>>>>> +      * cloned bio must not modify vec list
>>>>> +      */
>>>>> +     if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>>>>> +             return;
>>>>> +
>>>>> +     if (bio->bi_vcnt > 0) {
>>>>
>>>> In patch 1 you introduce bio_segments_all() with the log message 'So
>>>> that we can replace the direct access to .bi_vcnt.' Here you introduce a
>>>> new direct access to it (plus the duplicated WARN_ON_ONCE()).
>>>>
>>>> Maybe use the helper directly here (I admit I haven't gone through the
>>>> whole series yet, so I can't see if the change is made later).
>>>
>>> Firstly MD does need one helper to remove the last added page, as you
>>> can see there are three such uses in patch3.
>>>
>>> Secondly both the two helpers will be changed once multipage bvec
>>> is supported, that means we have to change MD too after multipage bvec
>>> if just using bio_segments_all() to replace .bi_vcnt for removing
>>> the last added page.
>>
>> I'm not sure if we're talking past each other here, I assumed you'd do
>> something like:
>>
>> void bio_remove_last_page(struct bio *bio)
>> {
>>         int vcnt = bio_segments_all(bio);
>>
>>         if (bio_flagged(bio, BIO_CLONED))
>>                 return;
>>
>>         if (vcnt > 0) {
>>                 struct bio_vec *bv = &bio->bi_io_vec[vcnt - 1];
>>
>>                 bio->bi_iter.bi_size -= bv->bv_len;
>>                 bio->bi_vcnt--;
>>         }
>> }
> 
> What we are doing is to remove the external direct access to bvec
> table in drivers or filesystems because they may misuse that, for
> example, drivers often use .bi_vcnt to get the page count in the bio,
> but the actual meaning is just bvec's count.
> 
> And the implementation in block layer still need to play the table directly,
> especially for sake of efficiency, cause we understand the details and won't
> misuse that.

Ah OK, so bio_segments_all() is intended for drivers.

Thanks for the clarification,
	Johannes
diff mbox

Patch

diff --git a/block/bio.c b/block/bio.c
index 5eec5e08417f..0ce7ffcd7939 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -837,6 +837,29 @@  int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
 EXPORT_SYMBOL(bio_add_pc_page);
 
 /**
+ *	bio_remove_last_page	-	remove the last added page
+ *	@bio: destination bio
+ *
+ *	Attempt to remove the last added page from the bio_vec maplist.
+ */
+void bio_remove_last_page(struct bio *bio)
+{
+	/*
+	 * cloned bio must not modify vec list
+	 */
+	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
+		return;
+
+	if (bio->bi_vcnt > 0) {
+		struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
+
+		bio->bi_iter.bi_size -= bv->bv_len;
+		bio->bi_vcnt--;
+	}
+}
+EXPORT_SYMBOL(bio_remove_last_page);
+
+/**
  *	bio_add_page	-	attempt to add page to bio
  *	@bio: destination bio
  *	@page: page to add
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 3364b3ed90e7..32aeb493d1fe 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -443,6 +443,7 @@  extern void bio_init(struct bio *bio, struct bio_vec *table,
 extern void bio_reset(struct bio *);
 void bio_chain(struct bio *, struct bio *);
 
+extern void bio_remove_last_page(struct bio *bio);
 extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
 extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *,
 			   unsigned int, unsigned int);