* [PATCH] xen/blkfront: correct purging of persistent grants
@ 2018-09-28 7:28 Juergen Gross
2018-09-28 7:58 ` [Xen-devel] " Alan Robinson
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Juergen Gross @ 2018-09-28 7:28 UTC (permalink / raw)
To: linux-kernel, xen-devel, linux-block
Cc: boris.ostrovsky, roger.pau, konrad.wilk, axboe, Juergen Gross
Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
stale persistent grants") introduced a regression as purged persistent
grants were not pu into the list of free grants again. Correct that.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
drivers/block/xen-blkfront.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index a71d817e900d..429d20131c7e 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2670,8 +2670,8 @@ static void purge_persistent_grants(struct blkfront_info *info)
list_del(&gnt_list_entry->node);
gnttab_end_foreign_access(gnt_list_entry->gref, 0, 0UL);
rinfo->persistent_gnts_c--;
- __free_page(gnt_list_entry->page);
- kfree(gnt_list_entry);
+ gnt_list_entry->gref = GRANT_INVALID_REF;
+ list_add_tail(&gnt_list_entry->node, &rinfo->grants);
}
spin_unlock_irqrestore(&rinfo->ring_lock, flags);
--
2.16.4
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Xen-devel] [PATCH] xen/blkfront: correct purging of persistent grants
2018-09-28 7:28 [PATCH] xen/blkfront: correct purging of persistent grants Juergen Gross
@ 2018-09-28 7:58 ` Alan Robinson
2018-09-28 12:45 ` Boris Ostrovsky
2018-09-28 15:41 ` Jens Axboe
2 siblings, 0 replies; 8+ messages in thread
From: Alan Robinson @ 2018-09-28 7:58 UTC (permalink / raw)
To: Juergen Gross
Cc: linux-kernel, xen-devel, linux-block, axboe, boris.ostrovsky,
konrad.wilk, roger.pau
[-- Attachment #1: Type: text/plain, Size: 299 bytes --]
On Fri, Sep 28, 2018 at 09:28:27AM +0200, Juergen Gross wrote:
> Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
> stale persistent grants") introduced a regression as purged persistent
> grants were not pu into the list of free grants again. Correct that.
s/pu /put /
Alan
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2642 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xen/blkfront: correct purging of persistent grants
2018-09-28 7:28 [PATCH] xen/blkfront: correct purging of persistent grants Juergen Gross
2018-09-28 7:58 ` [Xen-devel] " Alan Robinson
@ 2018-09-28 12:45 ` Boris Ostrovsky
2018-09-28 13:13 ` Juergen Gross
2018-09-28 15:41 ` Jens Axboe
2 siblings, 1 reply; 8+ messages in thread
From: Boris Ostrovsky @ 2018-09-28 12:45 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, linux-block
Cc: roger.pau, konrad.wilk, axboe
On 9/28/18 3:28 AM, Juergen Gross wrote:
> Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
> stale persistent grants") introduced a regression as purged persistent
> grants were not pu into the list of free grants again. Correct that.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> drivers/block/xen-blkfront.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index a71d817e900d..429d20131c7e 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -2670,8 +2670,8 @@ static void purge_persistent_grants(struct blkfront_info *info)
> list_del(&gnt_list_entry->node);
> gnttab_end_foreign_access(gnt_list_entry->gref, 0, 0UL);
> rinfo->persistent_gnts_c--;
> - __free_page(gnt_list_entry->page);
> - kfree(gnt_list_entry);
> + gnt_list_entry->gref = GRANT_INVALID_REF;
> + list_add_tail(&gnt_list_entry->node, &rinfo->grants);
Sorry, I don't follow this. What is the purpose of removing the grant
from rinfo->grants list with list_del() and then adding it back with
list_add_tail()?
-boris
> }
>
> spin_unlock_irqrestore(&rinfo->ring_lock, flags);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xen/blkfront: correct purging of persistent grants
2018-09-28 12:45 ` Boris Ostrovsky
@ 2018-09-28 13:13 ` Juergen Gross
2018-09-28 13:33 ` Boris Ostrovsky
0 siblings, 1 reply; 8+ messages in thread
From: Juergen Gross @ 2018-09-28 13:13 UTC (permalink / raw)
To: Boris Ostrovsky, linux-kernel, xen-devel, linux-block
Cc: roger.pau, konrad.wilk, axboe
On 28/09/2018 14:45, Boris Ostrovsky wrote:
> On 9/28/18 3:28 AM, Juergen Gross wrote:
>> Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
>> stale persistent grants") introduced a regression as purged persistent
>> grants were not pu into the list of free grants again. Correct that.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> drivers/block/xen-blkfront.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>> index a71d817e900d..429d20131c7e 100644
>> --- a/drivers/block/xen-blkfront.c
>> +++ b/drivers/block/xen-blkfront.c
>> @@ -2670,8 +2670,8 @@ static void purge_persistent_grants(struct blkfront_info *info)
>> list_del(&gnt_list_entry->node);
>> gnttab_end_foreign_access(gnt_list_entry->gref, 0, 0UL);
>> rinfo->persistent_gnts_c--;
>> - __free_page(gnt_list_entry->page);
>> - kfree(gnt_list_entry);
>> + gnt_list_entry->gref = GRANT_INVALID_REF;
>> + list_add_tail(&gnt_list_entry->node, &rinfo->grants);
>
> Sorry, I don't follow this. What is the purpose of removing the grant
> from rinfo->grants list with list_del() and then adding it back with
> list_add_tail()?
The persistent grants are at the list head and the non-persistent ones
at the tail.
Juergen
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xen/blkfront: correct purging of persistent grants
2018-09-28 13:13 ` Juergen Gross
@ 2018-09-28 13:33 ` Boris Ostrovsky
2018-09-28 13:52 ` Juergen Gross
0 siblings, 1 reply; 8+ messages in thread
From: Boris Ostrovsky @ 2018-09-28 13:33 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, linux-block
Cc: roger.pau, konrad.wilk, axboe
On 9/28/18 9:13 AM, Juergen Gross wrote:
> On 28/09/2018 14:45, Boris Ostrovsky wrote:
>> On 9/28/18 3:28 AM, Juergen Gross wrote:
>>> Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
>>> stale persistent grants") introduced a regression as purged persistent
>>> grants were not pu into the list of free grants again. Correct that.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> drivers/block/xen-blkfront.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>> index a71d817e900d..429d20131c7e 100644
>>> --- a/drivers/block/xen-blkfront.c
>>> +++ b/drivers/block/xen-blkfront.c
>>> @@ -2670,8 +2670,8 @@ static void purge_persistent_grants(struct blkfront_info *info)
>>> list_del(&gnt_list_entry->node);
>>> gnttab_end_foreign_access(gnt_list_entry->gref, 0, 0UL);
>>> rinfo->persistent_gnts_c--;
>>> - __free_page(gnt_list_entry->page);
>>> - kfree(gnt_list_entry);
>>> + gnt_list_entry->gref = GRANT_INVALID_REF;
>>> + list_add_tail(&gnt_list_entry->node, &rinfo->grants);
>> Sorry, I don't follow this. What is the purpose of removing the grant
>> from rinfo->grants list with list_del() and then adding it back with
>> list_add_tail()?
> The persistent grants are at the list head and the non-persistent ones
> at the tail.
Oh, I didn't realize that. But isn't that an optimization (and so not
following this rule should not cause errors)?
-boris
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xen/blkfront: correct purging of persistent grants
2018-09-28 13:33 ` Boris Ostrovsky
@ 2018-09-28 13:52 ` Juergen Gross
2018-09-28 14:03 ` Boris Ostrovsky
0 siblings, 1 reply; 8+ messages in thread
From: Juergen Gross @ 2018-09-28 13:52 UTC (permalink / raw)
To: Boris Ostrovsky, linux-kernel, xen-devel, linux-block
Cc: roger.pau, konrad.wilk, axboe
On 28/09/2018 15:33, Boris Ostrovsky wrote:
> On 9/28/18 9:13 AM, Juergen Gross wrote:
>> On 28/09/2018 14:45, Boris Ostrovsky wrote:
>>> On 9/28/18 3:28 AM, Juergen Gross wrote:
>>>> Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
>>>> stale persistent grants") introduced a regression as purged persistent
>>>> grants were not pu into the list of free grants again. Correct that.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> drivers/block/xen-blkfront.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>>> index a71d817e900d..429d20131c7e 100644
>>>> --- a/drivers/block/xen-blkfront.c
>>>> +++ b/drivers/block/xen-blkfront.c
>>>> @@ -2670,8 +2670,8 @@ static void purge_persistent_grants(struct blkfront_info *info)
>>>> list_del(&gnt_list_entry->node);
>>>> gnttab_end_foreign_access(gnt_list_entry->gref, 0, 0UL);
>>>> rinfo->persistent_gnts_c--;
>>>> - __free_page(gnt_list_entry->page);
>>>> - kfree(gnt_list_entry);
>>>> + gnt_list_entry->gref = GRANT_INVALID_REF;
>>>> + list_add_tail(&gnt_list_entry->node, &rinfo->grants);
>>> Sorry, I don't follow this. What is the purpose of removing the grant
>>> from rinfo->grants list with list_del() and then adding it back with
>>> list_add_tail()?
>> The persistent grants are at the list head and the non-persistent ones
>> at the tail.
>
> Oh, I didn't realize that. But isn't that an optimization (and so not
> following this rule should not cause errors)?
In theory: yes.
The persistent grant handling is rather complicated so I'd like to make
sure not to deviate from the common standards.
When I find some time I want to modify the persistent grant handling to
be more explicit and controlled completely by the frontend (within the
backend defined limits, of course). Until then we should try to modify
persistent grants not too much IMO.
Juergen
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xen/blkfront: correct purging of persistent grants
2018-09-28 13:52 ` Juergen Gross
@ 2018-09-28 14:03 ` Boris Ostrovsky
0 siblings, 0 replies; 8+ messages in thread
From: Boris Ostrovsky @ 2018-09-28 14:03 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, linux-block
Cc: roger.pau, konrad.wilk, axboe
On 9/28/18 9:52 AM, Juergen Gross wrote:
> On 28/09/2018 15:33, Boris Ostrovsky wrote:
>> On 9/28/18 9:13 AM, Juergen Gross wrote:
>>> On 28/09/2018 14:45, Boris Ostrovsky wrote:
>>>> On 9/28/18 3:28 AM, Juergen Gross wrote:
>>>>> Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
>>>>> stale persistent grants") introduced a regression as purged persistent
>>>>> grants were not pu into the list of free grants again. Correct that.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>> drivers/block/xen-blkfront.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>>>> index a71d817e900d..429d20131c7e 100644
>>>>> --- a/drivers/block/xen-blkfront.c
>>>>> +++ b/drivers/block/xen-blkfront.c
>>>>> @@ -2670,8 +2670,8 @@ static void purge_persistent_grants(struct blkfront_info *info)
>>>>> list_del(&gnt_list_entry->node);
>>>>> gnttab_end_foreign_access(gnt_list_entry->gref, 0, 0UL);
>>>>> rinfo->persistent_gnts_c--;
>>>>> - __free_page(gnt_list_entry->page);
>>>>> - kfree(gnt_list_entry);
>>>>> + gnt_list_entry->gref = GRANT_INVALID_REF;
>>>>> + list_add_tail(&gnt_list_entry->node, &rinfo->grants);
>>>> Sorry, I don't follow this. What is the purpose of removing the grant
>>>> from rinfo->grants list with list_del() and then adding it back with
>>>> list_add_tail()?
>>> The persistent grants are at the list head and the non-persistent ones
>>> at the tail.
>> Oh, I didn't realize that. But isn't that an optimization (and so not
>> following this rule should not cause errors)?
> In theory: yes.
>
> The persistent grant handling is rather complicated so I'd like to make
> sure not to deviate from the common standards.
I am not arguing with correctness of your patch, so
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
but I am a little surprised that it fixes Sander's problem.
-boris
>
> When I find some time I want to modify the persistent grant handling to
> be more explicit and controlled completely by the frontend (within the
> backend defined limits, of course). Until then we should try to modify
> persistent grants not too much IMO.
>
>
> Juergen
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen/blkfront: correct purging of persistent grants
2018-09-28 7:28 [PATCH] xen/blkfront: correct purging of persistent grants Juergen Gross
2018-09-28 7:58 ` [Xen-devel] " Alan Robinson
2018-09-28 12:45 ` Boris Ostrovsky
@ 2018-09-28 15:41 ` Jens Axboe
2 siblings, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2018-09-28 15:41 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel, linux-block
Cc: boris.ostrovsky, roger.pau, konrad.wilk
On 9/28/18 1:28 AM, Juergen Gross wrote:
> Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
> stale persistent grants") introduced a regression as purged persistent
> grants were not pu into the list of free grants again. Correct that.
I'll apply this for 4.19, and if things unexpectedly don't work out,
we can always revert the series next week.
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-09-28 15:41 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-28 7:28 [PATCH] xen/blkfront: correct purging of persistent grants Juergen Gross
2018-09-28 7:58 ` [Xen-devel] " Alan Robinson
2018-09-28 12:45 ` Boris Ostrovsky
2018-09-28 13:13 ` Juergen Gross
2018-09-28 13:33 ` Boris Ostrovsky
2018-09-28 13:52 ` Juergen Gross
2018-09-28 14:03 ` Boris Ostrovsky
2018-09-28 15:41 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome