[PATCH] frontswap: enable call to invalidate area on swapoff

[PATCH] frontswap: enable call to invalidate area on swapoff

[Krzysztof Kozlowski]

During swapoff the frontswap_map was NULL-ified before calling
frontswap_invalidate_area(). However the frontswap_invalidate_area()
exits early if frontswap_map is NULL. Invalidate was never called during
swapoff.

This patch moves frontswap_map_set() in swapoff just after calling
frontswap_invalidate_area() so outside of locks
(swap_lock and swap_info_struct->lock). This shouldn't be a problem as
during swapon the frontswap_map_set() is called also outside of any
locks.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 mm/swapfile.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 3963fc2..3a4896b 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1922,10 +1922,10 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
 	p->cluster_info = NULL;
 	p->flags = 0;
 	frontswap_map = frontswap_map_get(p);
-	frontswap_map_set(p, NULL);
 	spin_unlock(&p->lock);
 	spin_unlock(&swap_lock);
 	frontswap_invalidate_area(type);
+	frontswap_map_set(p, NULL);
 	mutex_unlock(&swapon_mutex);
 	free_percpu(p->percpu_cluster);
 	p->percpu_cluster = NULL;
-- 
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Seth Jennings]

On Mon, Oct 07, 2013 at 05:25:41PM +0200, Krzysztof Kozlowski wrote:
> During swapoff the frontswap_map was NULL-ified before calling
> frontswap_invalidate_area(). However the frontswap_invalidate_area()
> exits early if frontswap_map is NULL. Invalidate was never called during
> swapoff.
> 
> This patch moves frontswap_map_set() in swapoff just after calling
> frontswap_invalidate_area() so outside of locks
> (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> during swapon the frontswap_map_set() is called also outside of any
> locks.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Nice catch!

Reviewed-by: Seth Jennings <sjenning@linux.vnet.ibm.com>

> ---
>  mm/swapfile.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3963fc2..3a4896b 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1922,10 +1922,10 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
>  	p->cluster_info = NULL;
>  	p->flags = 0;
>  	frontswap_map = frontswap_map_get(p);
> -	frontswap_map_set(p, NULL);
>  	spin_unlock(&p->lock);
>  	spin_unlock(&swap_lock);
>  	frontswap_invalidate_area(type);
> +	frontswap_map_set(p, NULL);
>  	mutex_unlock(&swapon_mutex);
>  	free_percpu(p->percpu_cluster);
>  	p->percpu_cluster = NULL;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Andrew Morton]

On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:

> During swapoff the frontswap_map was NULL-ified before calling
> frontswap_invalidate_area(). However the frontswap_invalidate_area()
> exits early if frontswap_map is NULL. Invalidate was never called during
> swapoff.
> 
> This patch moves frontswap_map_set() in swapoff just after calling
> frontswap_invalidate_area() so outside of locks
> (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> during swapon the frontswap_map_set() is called also outside of any
> locks.
> 

Ahem.  So there's a bunch of code in __frontswap_invalidate_area()
which hasn't ever been executed and nobody noticed it.  So perhaps that
code isn't actually needed?

More seriously, this patch looks like it enables code which hasn't been
used or tested before.  How well tested was this?

Are there any runtime-visible effects from this change?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Krzysztof Kozlowski]

On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
> On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 
> > During swapoff the frontswap_map was NULL-ified before calling
> > frontswap_invalidate_area(). However the frontswap_invalidate_area()
> > exits early if frontswap_map is NULL. Invalidate was never called during
> > swapoff.
> > 
> > This patch moves frontswap_map_set() in swapoff just after calling
> > frontswap_invalidate_area() so outside of locks
> > (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> > during swapon the frontswap_map_set() is called also outside of any
> > locks.
> > 
> 
> Ahem.  So there's a bunch of code in __frontswap_invalidate_area()
> which hasn't ever been executed and nobody noticed it.  So perhaps that
> code isn't actually needed?
> 
> More seriously, this patch looks like it enables code which hasn't been
> used or tested before.  How well tested was this?
> 
> Are there any runtime-visible effects from this change?

I tested zswap on x86 and x86-64 and there was no difference. This is
good as there shouldn't be visible anything because swapoff is unusing
all pages anyway:
	try_to_unuse(type, false, 0); /* force all pages to be unused */

I haven't tested other frontswap users.


Best regards,
Krzysztof Kozlowski



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Andrew Morton]

On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:

> On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
> > On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> > 
> > > During swapoff the frontswap_map was NULL-ified before calling
> > > frontswap_invalidate_area(). However the frontswap_invalidate_area()
> > > exits early if frontswap_map is NULL. Invalidate was never called during
> > > swapoff.
> > > 
> > > This patch moves frontswap_map_set() in swapoff just after calling
> > > frontswap_invalidate_area() so outside of locks
> > > (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> > > during swapon the frontswap_map_set() is called also outside of any
> > > locks.
> > > 
> > 
> > Ahem.  So there's a bunch of code in __frontswap_invalidate_area()
> > which hasn't ever been executed and nobody noticed it.  So perhaps that
> > code isn't actually needed?
> > 
> > More seriously, this patch looks like it enables code which hasn't been
> > used or tested before.  How well tested was this?
> > 
> > Are there any runtime-visible effects from this change?
> 
> I tested zswap on x86 and x86-64 and there was no difference. This is
> good as there shouldn't be visible anything because swapoff is unusing
> all pages anyway:
> 	try_to_unuse(type, false, 0); /* force all pages to be unused */
> 
> I haven't tested other frontswap users.

So is that code in __frontswap_invalidate_area() unneeded?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Bob Liu]

On 10/09/2013 04:08 AM, Andrew Morton wrote:
> On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 
>> On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
>>> On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
>>>
>>>> During swapoff the frontswap_map was NULL-ified before calling
>>>> frontswap_invalidate_area(). However the frontswap_invalidate_area()
>>>> exits early if frontswap_map is NULL. Invalidate was never called during
>>>> swapoff.
>>>>
>>>> This patch moves frontswap_map_set() in swapoff just after calling
>>>> frontswap_invalidate_area() so outside of locks
>>>> (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
>>>> during swapon the frontswap_map_set() is called also outside of any
>>>> locks.
>>>>
>>>
>>> Ahem.  So there's a bunch of code in __frontswap_invalidate_area()
>>> which hasn't ever been executed and nobody noticed it.  So perhaps that
>>> code isn't actually needed?
>>>
>>> More seriously, this patch looks like it enables code which hasn't been
>>> used or tested before.  How well tested was this?
>>>
>>> Are there any runtime-visible effects from this change?
>>
>> I tested zswap on x86 and x86-64 and there was no difference. This is
>> good as there shouldn't be visible anything because swapoff is unusing
>> all pages anyway:
>> 	try_to_unuse(type, false, 0); /* force all pages to be unused */
>>
>> I haven't tested other frontswap users.
> 
> So is that code in __frontswap_invalidate_area() unneeded?
> 

I don't think so, it's still needed otherwise there will be memory leak.
I'm afraid nobody noticed the memory leak here before, this patch can
fix it. Sorry for didn't pay enough attention but please keep
__frontswap_invalidate_area().

-- 
Regards,
-Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Seth Jennings]

On Tue, Oct 08, 2013 at 01:08:53PM -0700, Andrew Morton wrote:
> On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 
> > On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
> > > On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> > > 
> > > > During swapoff the frontswap_map was NULL-ified before calling
> > > > frontswap_invalidate_area(). However the frontswap_invalidate_area()
> > > > exits early if frontswap_map is NULL. Invalidate was never called during
> > > > swapoff.
> > > > 
> > > > This patch moves frontswap_map_set() in swapoff just after calling
> > > > frontswap_invalidate_area() so outside of locks
> > > > (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
> > > > during swapon the frontswap_map_set() is called also outside of any
> > > > locks.
> > > > 
> > > 
> > > Ahem.  So there's a bunch of code in __frontswap_invalidate_area()
> > > which hasn't ever been executed and nobody noticed it.  So perhaps that
> > > code isn't actually needed?
> > > 
> > > More seriously, this patch looks like it enables code which hasn't been
> > > used or tested before.  How well tested was this?
> > > 
> > > Are there any runtime-visible effects from this change?
> > 
> > I tested zswap on x86 and x86-64 and there was no difference. This is
> > good as there shouldn't be visible anything because swapoff is unusing
> > all pages anyway:
> > 	try_to_unuse(type, false, 0); /* force all pages to be unused */
> > 
> > I haven't tested other frontswap users.
> 
> So is that code in __frontswap_invalidate_area() unneeded?

Yes, to expand on what Bob said, __frontswap_invalidate_area() is still
needed to let any frontswap backend free per-swaptype resources.

__frontswap_invalidate_area() is _not_ for freeing structures associated
with individual swapped out pages since all of the pages should be
brought back into memory by try_to_unuse() before
__frontswap_invalidate_area() is called.

The reason we never noticed this for zswap is that zswap has no
dynamically allocated per-type resources.  In the expected case,
where all of the pages have been drained from zswap,
zswap_frontswap_invalidate_area() is a no-op.

Seth

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Bob Liu]

On 10/09/2013 10:40 PM, Seth Jennings wrote:
> On Tue, Oct 08, 2013 at 01:08:53PM -0700, Andrew Morton wrote:
>> On Tue, 08 Oct 2013 10:13:20 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
>>
>>> On pon, 2013-10-07 at 15:03 -0700, Andrew Morton wrote:
>>>> On Mon, 07 Oct 2013 17:25:41 +0200 Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
>>>>
>>>>> During swapoff the frontswap_map was NULL-ified before calling
>>>>> frontswap_invalidate_area(). However the frontswap_invalidate_area()
>>>>> exits early if frontswap_map is NULL. Invalidate was never called during
>>>>> swapoff.
>>>>>
>>>>> This patch moves frontswap_map_set() in swapoff just after calling
>>>>> frontswap_invalidate_area() so outside of locks
>>>>> (swap_lock and swap_info_struct->lock). This shouldn't be a problem as
>>>>> during swapon the frontswap_map_set() is called also outside of any
>>>>> locks.
>>>>>
>>>>
>>>> Ahem.  So there's a bunch of code in __frontswap_invalidate_area()
>>>> which hasn't ever been executed and nobody noticed it.  So perhaps that
>>>> code isn't actually needed?
>>>>
>>>> More seriously, this patch looks like it enables code which hasn't been
>>>> used or tested before.  How well tested was this?
>>>>
>>>> Are there any runtime-visible effects from this change?
>>>
>>> I tested zswap on x86 and x86-64 and there was no difference. This is
>>> good as there shouldn't be visible anything because swapoff is unusing
>>> all pages anyway:
>>> 	try_to_unuse(type, false, 0); /* force all pages to be unused */
>>>
>>> I haven't tested other frontswap users.
>>
>> So is that code in __frontswap_invalidate_area() unneeded?
> 
> Yes, to expand on what Bob said, __frontswap_invalidate_area() is still
> needed to let any frontswap backend free per-swaptype resources.
> 
> __frontswap_invalidate_area() is _not_ for freeing structures associated
> with individual swapped out pages since all of the pages should be
> brought back into memory by try_to_unuse() before
> __frontswap_invalidate_area() is called.
> 
> The reason we never noticed this for zswap is that zswap has no
> dynamically allocated per-type resources.  In the expected case,
> where all of the pages have been drained from zswap,
> zswap_frontswap_invalidate_area() is a no-op.
> 

Not exactly, see the bug fix "mm/zswap: bugfix: memory leak when
re-swapon" from Weijie.
Zswap needs invalidate_area() also.

Thanks,
-Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Seth Jennings]

On Thu, Oct 10, 2013 at 09:29:07AM +0800, Bob Liu wrote:
> On 10/09/2013 10:40 PM, Seth Jennings wrote:
> > 
> > The reason we never noticed this for zswap is that zswap has no
> > dynamically allocated per-type resources.  In the expected case,
> > where all of the pages have been drained from zswap,
> > zswap_frontswap_invalidate_area() is a no-op.
> > 
> 
> Not exactly, see the bug fix "mm/zswap: bugfix: memory leak when
> re-swapon" from Weijie.
> Zswap needs invalidate_area() also.

I remembered this patch as soon as I sent out this note.  What I said
about zswap_frontswap_invalidate_area() being a no-op in the expected
case is true as of v3.12-rc4, but it shouldn't be :)

I sent a note to Andrew reminding him to pull in that patch.

Thanks,
Seth

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Weijie Yang]

Thanks, Seth

On Thu, Oct 10, 2013 at 10:26 AM, Seth Jennings
<sjennings@variantweb.net> wrote:
> On Thu, Oct 10, 2013 at 09:29:07AM +0800, Bob Liu wrote:
>> On 10/09/2013 10:40 PM, Seth Jennings wrote:
>> >
>> > The reason we never noticed this for zswap is that zswap has no
>> > dynamically allocated per-type resources.  In the expected case,
>> > where all of the pages have been drained from zswap,
>> > zswap_frontswap_invalidate_area() is a no-op.
>> >
>>
>> Not exactly, see the bug fix "mm/zswap: bugfix: memory leak when
>> re-swapon" from Weijie.
>> Zswap needs invalidate_area() also.
>
> I remembered this patch as soon as I sent out this note.  What I said
> about zswap_frontswap_invalidate_area() being a no-op in the expected
> case is true as of v3.12-rc4, but it shouldn't be :)
>
> I sent a note to Andrew reminding him to pull in that patch.
>
> Thanks,
> Seth
> --
> 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/

I am sorry to interrupt this topic, but I found an tiny issue near that:

we can not "set_blocksize(bdev, p->old_block_size);" at the end of swapoff()
because swap_info p may be reused by concurrent swapon called
I think we need to  save the p->old_block_size in a local var and use it instead

to Krzysztof : would you please add this in your patch?
Thanks

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Krzysztof Kozlowski]

On Fri, 2013-10-11 at 10:23 +0800, Weijie Yang wrote:
> I am sorry to interrupt this topic, but I found an tiny issue near that:
> 
> we can not "set_blocksize(bdev, p->old_block_size);" at the end of swapoff()
> because swap_info p may be reused by concurrent swapon called
> I think we need to  save the p->old_block_size in a local var and use it instead
I confirm the race here (I was able to trigger it on the same swap type).


> to Krzysztof : would you please add this in your patch?
> Thanks
I think it should be another patch as this is not related with
frontswap. I'll send new one and add you as Reported-by. Is it OK with
you?


Best regards,
Krzysztof

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] frontswap: enable call to invalidate area on swapoff

[Weijie Yang]

On Fri, Oct 11, 2013 at 5:25 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On Fri, 2013-10-11 at 10:23 +0800, Weijie Yang wrote:
>> I am sorry to interrupt this topic, but I found an tiny issue near that:
>>
>> we can not "set_blocksize(bdev, p->old_block_size);" at the end of swapoff()
>> because swap_info p may be reused by concurrent swapon called
>> I think we need to  save the p->old_block_size in a local var and use it instead
> I confirm the race here (I was able to trigger it on the same swap type).
>
>
>> to Krzysztof : would you please add this in your patch?
>> Thanks
> I think it should be another patch as this is not related with
> frontswap. I'll send new one and add you as Reported-by. Is it OK with
> you?

All right.

>
> Best regards,
> Krzysztof
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>