Skip to contentSkip to conversation

Questin about swap_slot free and invalidate page

DISCUSSION Minchan Kim 4 participants 11 messages Jan 31, 2013 – Feb 25, 2013

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

When I reviewed zswap, I was curious about frontswap_store. It said following as.

* If frontswap already contains a page with matching swaptype and
* offset, the frontswap implementation may either overwrite the data and
* return success or invalidate the page from frontswap and return failure.

It didn't say why it happens. we already have __frontswap_invalidate_page and call it whenever swap_slot frees. If we don't free swap slot, scan_swap_map can't find the slot for swap out so I thought overwriting of data shouldn't happen in frontswap.

As I looked the code, the curplit is reuse_swap_page. It couldn't free swap slot if the page founded is PG_writeback but miss calling frontswap_invalidate_page so data overwriting on frontswap can happen. I'm not sure frontswap guys already discussed it long time ago.

If we can fix it, we can remove duplication entry handling logic in all of backend of frontswap. All of backend should handle it although it's pretty rare. Of course, zram could be fixed. It might be trivial now but more there are many backend of frontswap, more it would be a headache.

If we are trying to fix it in swap layer, we might fix it following as

int reuse_swap_page(struct page *page)

{
        ..
        ..
        if (count == 1) {
                if (!PageWriteback(page)) {
                        delete_from_swap_cache(page);
                        SetPageDirty(page);
                } else {
                        frontswap_invalidate_page();
                        swap_slot_free_notify();
                }
        }
}

But not sure, it is worth at the moment and there might be other places to be fixed.(I hope Hugh can point out if we are missing something if he has a time)

If we are reluctant to it, at least, we should write out comment above frontswap_store about that to notice curious guys who spend many time to know WHY and smart guys who are going to fix it with nice way.

Mr. Frontswap, What do you think about it?

Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#1 lore raw
Quoted 39 lines · Minchan Kim

When I reviewed zswap, I was curious about frontswap_store. It said following as.

* If frontswap already contains a page with matching swaptype and
* offset, the frontswap implementation may either overwrite the data and
* return success or invalidate the page from frontswap and return failure.

It didn't say why it happens. we already have __frontswap_invalidate_page and call it whenever swap_slot frees. If we don't free swap slot, scan_swap_map can't find the slot for swap out so I thought overwriting of data shouldn't happen in frontswap.

As I looked the code, the curplit is reuse_swap_page. It couldn't free swap slot if the page founded is PG_writeback but miss calling frontswap_invalidate_page so data overwriting on frontswap can happen. I'm not sure frontswap guys already discussed it long time ago.

If we can fix it, we can remove duplication entry handling logic in all of backend of frontswap. All of backend should handle it although it's pretty rare. Of course, zram could be fixed. It might be trivial now but more there are many backend of frontswap, more it would be a headache.

If we are trying to fix it in swap layer, we might fix it following as

int reuse_swap_page(struct page *page)

{
        ..
        ..
        if (count == 1) {
                if (!PageWriteback(page)) {
                        delete_from_swap_cache(page);
                        SetPageDirty(page);
                } else {
                        frontswap_invalidate_page();
                        swap_slot_free_notify();
                }
        }
}
Quoted 3 lines

But not sure, it is worth at the moment and there might be other places to be fixed.(I hope Hugh can point out if we are missing something if he has a time)

I expect you are right that reuse_swap_page() is the only way it would happen for frontswap; but I'm too unfamiliar with frontswap to promise you that - it's better that you insert WARN_ONs in your testing to verify.

But I think it's a general tmem property, isn't it? To define what happens if you do give it the same key again. So I doubt it's something that has to be fixed; but if you do find it helpful to fix it, bear in mind that reuse_swap_page() is an odd corner, which may one day give the "stable pages" DIF/DIX people trouble, though they've not yet complained.

I'd prefer a patch not specific to frontswap, but along the lines below:
I think that's the most robust way to express it, though I don't think the (count == 0) case can actually occur inside that block (whereas count == 0 certainly can occur in the !PageSwapCache case).

I believe that I once upon a time took statistics of how often the PageWriteback case happens here, and concluded that it wasn't often enough that refusing to reuse in this case would be likely to slow anyone down noticeably.

Quoted 6 lines

If we are reluctant to it, at least, we should write out comment above frontswap_store about that to notice curious guys who spend many time to know WHY and smart guys who are going to fix it with nice way.

Mr. Frontswap, What do you think about it?

He's not me of course :)

Hugh

mm/swapfile.c +5 -2
@@ -637,8 +637,11 @@ int reuse_swap_page(struct page *page)
 		return 0;
 	count = page_mapcount(page);
 	if (count <= 1 && PageSwapCache(page)) {
-		count += page_swapcount(page);
-		if (count == 1 && !PageWriteback(page)) {
+		if (PageWriteback(page))
+			count = 2;	/* not safe yet to free its swap */
+		else
+			count += page_swapcount(page);
+		if (count <= 1) {
 			delete_from_swap_cache(page);
 			SetPageDirty(page);
 		}

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

#2 lore raw

Hi Hugh,

Quoted 45 lines · Hugh Dickins
Quoted 39 lines · Minchan Kim

When I reviewed zswap, I was curious about frontswap_store. It said following as.

* If frontswap already contains a page with matching swaptype and
* offset, the frontswap implementation may either overwrite the data and
* return success or invalidate the page from frontswap and return failure.

It didn't say why it happens. we already have __frontswap_invalidate_page and call it whenever swap_slot frees. If we don't free swap slot, scan_swap_map can't find the slot for swap out so I thought overwriting of data shouldn't happen in frontswap.

As I looked the code, the curplit is reuse_swap_page. It couldn't free swap slot if the page founded is PG_writeback but miss calling frontswap_invalidate_page so data overwriting on frontswap can happen. I'm not sure frontswap guys already discussed it long time ago.

If we can fix it, we can remove duplication entry handling logic in all of backend of frontswap. All of backend should handle it although it's pretty rare. Of course, zram could be fixed. It might be trivial now but more there are many backend of frontswap, more it would be a headache.

If we are trying to fix it in swap layer, we might fix it following as

int reuse_swap_page(struct page *page)

{
        ..
        ..
        if (count == 1) {
                if (!PageWriteback(page)) {
                        delete_from_swap_cache(page);
                        SetPageDirty(page);
                } else {
                        frontswap_invalidate_page();
                        swap_slot_free_notify();
                }
        }
}
Quoted 3 lines

But not sure, it is worth at the moment and there might be other places to be fixed.(I hope Hugh can point out if we are missing something if he has a time)

Quoted 6 lines

I expect you are right that reuse_swap_page() is the only way it would happen for frontswap; but I'm too unfamiliar with frontswap to promise you that - it's better that you insert WARN_ONs in your testing to verify.

But I think it's a general tmem property, isn't it? To define what happens if you do give it the same key again. So I doubt it's something

I am too unfamiliar with tmem property but thing I am seeing is EXPORT_SYMBOL(__frontswap_store). It's a one of frontend and is tighly very coupled with swap subsystem.

Quoted 9 lines

that has to be fixed; but if you do find it helpful to fix it, bear in mind that reuse_swap_page() is an odd corner, which may one day give the "stable pages" DIF/DIX people trouble, though they've not yet complained.

I'd prefer a patch not specific to frontswap, but along the lines below:
I think that's the most robust way to express it, though I don't think the (count == 0) case can actually occur inside that block (whereas count == 0 certainly can occur in the !PageSwapCache case).

Quoted 4 lines

I believe that I once upon a time took statistics of how often the PageWriteback case happens here, and concluded that it wasn't often enough that refusing to reuse in this case would be likely to slow anyone down noticeably.

I agree. I had a test about that with zram and that case wasn't common. so your patch looks good to me.

I am waiting Dan's reply(He will come in this week) and then, judge what's the best.

Thanks!

Quoted 34 lines
Quoted 7 lines

If we are reluctant to it, at least, we should write out comment above frontswap_store about that to notice curious guys who spend many time to know WHY and smart guys who are going to fix it with nice way.

Mr. Frontswap, What do you think about it?

He's not me of course :)

Hugh

mm/swapfile.c +5 -3
@@ -637,8 +637,11 @@ int reuse_swap_page(struct page *page)
 		return 0;
 	count = page_mapcount(page);
 	if (count <= 1 && PageSwapCache(page)) {
-		count += page_swapcount(page);
-		if (count == 1 && !PageWriteback(page)) {
+		if (PageWriteback(page))
+			count = 2;	/* not safe yet to free its swap */
+		else
+			count += page_swapcount(page);
+		if (count <= 1) {
 			delete_from_swap_cache(page);
 			SetPageDirty(page);
 		}
 
--

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

Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#3 lore raw
Quoted 80 lines

From: Minchan Kim [mailto:[email protected]]
Sent: Sunday, February 03, 2013 7:50 PM
To: Hugh Dickins
Cc: Nitin Gupta; Dan Magenheimer; Seth Jennings; Konrad Rzeszutek Wilk; [email protected]; linux- [email protected]; Andrew Morton
Subject: Re: Questin about swap_slot free and invalidate page

Hi Hugh,

Quoted 45 lines · Hugh Dickins
Quoted 39 lines · Minchan Kim

When I reviewed zswap, I was curious about frontswap_store. It said following as.

* If frontswap already contains a page with matching swaptype and
* offset, the frontswap implementation may either overwrite the data and
* return success or invalidate the page from frontswap and return failure.

It didn't say why it happens. we already have __frontswap_invalidate_page and call it whenever swap_slot frees. If we don't free swap slot, scan_swap_map can't find the slot for swap out so I thought overwriting of data shouldn't happen in frontswap.

As I looked the code, the curplit is reuse_swap_page. It couldn't free swap slot if the page founded is PG_writeback but miss calling frontswap_invalidate_page so data overwriting on frontswap can happen. I'm not sure frontswap guys already discussed it long time ago.

If we can fix it, we can remove duplication entry handling logic in all of backend of frontswap. All of backend should handle it although it's pretty rare. Of course, zram could be fixed. It might be trivial now but more there are many backend of frontswap, more it would be a headache.

If we are trying to fix it in swap layer, we might fix it following as

int reuse_swap_page(struct page *page)

{
        ..
        ..
        if (count == 1) {
                if (!PageWriteback(page)) {
                        delete_from_swap_cache(page);
                        SetPageDirty(page);
                } else {
                        frontswap_invalidate_page();
                        swap_slot_free_notify();
                }
        }
}
Quoted 3 lines

But not sure, it is worth at the moment and there might be other places to be fixed.(I hope Hugh can point out if we are missing something if he has a time)

Quoted 6 lines

I expect you are right that reuse_swap_page() is the only way it would happen for frontswap; but I'm too unfamiliar with frontswap to promise you that - it's better that you insert WARN_ONs in your testing to verify.

But I think it's a general tmem property, isn't it? To define what happens if you do give it the same key again. So I doubt it's something

I am too unfamiliar with tmem property but thing I am seeing is EXPORT_SYMBOL(__frontswap_store). It's a one of frontend and is tighly very coupled with swap subsystem.

Quoted 9 lines

that has to be fixed; but if you do find it helpful to fix it, bear in mind that reuse_swap_page() is an odd corner, which may one day give the "stable pages" DIF/DIX people trouble, though they've not yet complained.

I'd prefer a patch not specific to frontswap, but along the lines below:
I think that's the most robust way to express it, though I don't think the (count == 0) case can actually occur inside that block (whereas count == 0 certainly can occur in the !PageSwapCache case).

Quoted 4 lines

I believe that I once upon a time took statistics of how often the PageWriteback case happens here, and concluded that it wasn't often enough that refusing to reuse in this case would be likely to slow anyone down noticeably.

Quoted 5 lines

I agree. I had a test about that with zram and that case wasn't common. so your patch looks good to me.

I am waiting Dan's reply(He will come in this week) and then, judge what's the best.

Hugh is right that handling the possibility of duplicates is part of the tmem ABI. If there is any possibility of duplicates, the ABI defines how a backend must handle them to avoid data coherency issues.

The kernel implements an in-kernel API which implements the tmem ABI. If the frontend and backend can always agree that duplicates are never possible, I agree that the backend could avoid that special case. However, duplicates occur rarely enough and the consequences (data loss) are bad enough that I think the case should still be checked, at least with a BUG_ON. I also wonder if it is worth it to make changes to the core swap subsystem to avoid code to implement a zswap corner case.

Remember that zswap is an oversimplified special case of tmem that handles only one frontend (Linux frontswap) and one backend (zswap). Tmem goes well beyond that and already supports other more general backends including Xen and ramster, and could also support other frontends such as a BSD or Solaris equivalent of frontswap, for example with a Linux ramster/zcache backend. I'm not sure how wise it is to tear out generic code and replace it with simplistic code unless there is absolutely no chance that the generic code will be necessary.

My two cents,
Dan

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

#4 lore raw
Quoted 91 lines · Dan Magenheimer
Quoted 80 lines

From: Minchan Kim [mailto:[email protected]]
Sent: Sunday, February 03, 2013 7:50 PM
To: Hugh Dickins
Cc: Nitin Gupta; Dan Magenheimer; Seth Jennings; Konrad Rzeszutek Wilk; [email protected]; linux- [email protected]; Andrew Morton
Subject: Re: Questin about swap_slot free and invalidate page

Hi Hugh,

Quoted 45 lines · Hugh Dickins
Quoted 39 lines · Minchan Kim

When I reviewed zswap, I was curious about frontswap_store. It said following as.

* If frontswap already contains a page with matching swaptype and
* offset, the frontswap implementation may either overwrite the data and
* return success or invalidate the page from frontswap and return failure.

It didn't say why it happens. we already have __frontswap_invalidate_page and call it whenever swap_slot frees. If we don't free swap slot, scan_swap_map can't find the slot for swap out so I thought overwriting of data shouldn't happen in frontswap.

As I looked the code, the curplit is reuse_swap_page. It couldn't free swap slot if the page founded is PG_writeback but miss calling frontswap_invalidate_page so data overwriting on frontswap can happen. I'm not sure frontswap guys already discussed it long time ago.

If we can fix it, we can remove duplication entry handling logic in all of backend of frontswap. All of backend should handle it although it's pretty rare. Of course, zram could be fixed. It might be trivial now but more there are many backend of frontswap, more it would be a headache.

If we are trying to fix it in swap layer, we might fix it following as

int reuse_swap_page(struct page *page)

{
        ..
        ..
        if (count == 1) {
                if (!PageWriteback(page)) {
                        delete_from_swap_cache(page);
                        SetPageDirty(page);
                } else {
                        frontswap_invalidate_page();
                        swap_slot_free_notify();
                }
        }
}
Quoted 3 lines

But not sure, it is worth at the moment and there might be other places to be fixed.(I hope Hugh can point out if we are missing something if he has a time)

Quoted 6 lines

I expect you are right that reuse_swap_page() is the only way it would happen for frontswap; but I'm too unfamiliar with frontswap to promise you that - it's better that you insert WARN_ONs in your testing to verify.

But I think it's a general tmem property, isn't it? To define what happens if you do give it the same key again. So I doubt it's something

I am too unfamiliar with tmem property but thing I am seeing is EXPORT_SYMBOL(__frontswap_store). It's a one of frontend and is tighly very coupled with swap subsystem.

Quoted 9 lines

that has to be fixed; but if you do find it helpful to fix it, bear in mind that reuse_swap_page() is an odd corner, which may one day give the "stable pages" DIF/DIX people trouble, though they've not yet complained.

I'd prefer a patch not specific to frontswap, but along the lines below:
I think that's the most robust way to express it, though I don't think the (count == 0) case can actually occur inside that block (whereas count == 0 certainly can occur in the !PageSwapCache case).

Quoted 4 lines

I believe that I once upon a time took statistics of how often the PageWriteback case happens here, and concluded that it wasn't often enough that refusing to reuse in this case would be likely to slow anyone down noticeably.

Quoted 5 lines

I agree. I had a test about that with zram and that case wasn't common. so your patch looks good to me.

I am waiting Dan's reply(He will come in this week) and then, judge what's the best.

Hugh is right that handling the possibility of duplicates is part of the tmem ABI. If there is any possibility of duplicates, the ABI defines how a backend must handle them to avoid data coherency issues.

Quoted 8 lines

The kernel implements an in-kernel API which implements the tmem ABI. If the frontend and backend can always agree that duplicates are never possible, I agree that the backend could avoid that special case. However, duplicates occur rarely enough and the consequences (data loss) are bad enough that I think the case should still be checked, at least with a BUG_ON. I also wonder if it is worth it to make changes to the core swap subsystem to avoid code to implement a zswap corner case.

It wasn't only zswap but it could be applied zram, too.

Quoted 10 lines

Remember that zswap is an oversimplified special case of tmem that handles only one frontend (Linux frontswap) and one backend (zswap). Tmem goes well beyond that and already supports other more general backends including Xen and ramster, and could also support other frontends such as a BSD or Solaris equivalent of frontswap, for example with a Linux ramster/zcache backend. I'm not sure how wise it is to tear out generic code and replace it with simplistic code unless there is absolutely no chance that the generic code will be necessary.

Fair enough.

Thanks for clarifying that, Hugh, Dan.

Quoted 9 lines

My two cents,
Dan

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

Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#5 lore raw
In reply to Dan Magenheimer
Quoted 82 lines · Dan Magenheimer
Quoted 77 lines

From: Minchan Kim [mailto:[email protected]]
Sent: Sunday, February 03, 2013 7:50 PM
To: Hugh Dickins
Cc: Nitin Gupta; Dan Magenheimer; Seth Jennings; Konrad Rzeszutek Wilk; [email protected]; linux- [email protected]; Andrew Morton
Subject: Re: Questin about swap_slot free and invalidate page

Hi Hugh,

Quoted 44 lines · Hugh Dickins
Quoted 39 lines · Minchan Kim

When I reviewed zswap, I was curious about frontswap_store. It said following as.

* If frontswap already contains a page with matching swaptype and
* offset, the frontswap implementation may either overwrite the data and
* return success or invalidate the page from frontswap and return failure.

It didn't say why it happens. we already have __frontswap_invalidate_page and call it whenever swap_slot frees. If we don't free swap slot, scan_swap_map can't find the slot for swap out so I thought overwriting of data shouldn't happen in frontswap.

As I looked the code, the curplit is reuse_swap_page. It couldn't free swap slot if the page founded is PG_writeback but miss calling frontswap_invalidate_page so data overwriting on frontswap can happen. I'm not sure frontswap guys already discussed it long time ago.

If we can fix it, we can remove duplication entry handling logic in all of backend of frontswap. All of backend should handle it although it's pretty rare. Of course, zram could be fixed. It might be trivial now but more there are many backend of frontswap, more it would be a headache.

If we are trying to fix it in swap layer, we might fix it following as

int reuse_swap_page(struct page *page)

{
         ..
         ..
         if (count == 1) {
                 if (!PageWriteback(page)) {
                         delete_from_swap_cache(page);
                         SetPageDirty(page);
                 } else {
                         frontswap_invalidate_page();
                         swap_slot_free_notify();
                 }
         }
}
Quoted 3 lines

But not sure, it is worth at the moment and there might be other places to be fixed.(I hope Hugh can point out if we are missing something if he has a time)

Quoted 6 lines

I expect you are right that reuse_swap_page() is the only way it would happen for frontswap; but I'm too unfamiliar with frontswap to promise you that - it's better that you insert WARN_ONs in your testing to verify.

But I think it's a general tmem property, isn't it? To define what happens if you do give it the same key again. So I doubt it's something

I am too unfamiliar with tmem property but thing I am seeing is EXPORT_SYMBOL(__frontswap_store). It's a one of frontend and is tighly very coupled with swap subsystem.

Quoted 9 lines

that has to be fixed; but if you do find it helpful to fix it, bear in mind that reuse_swap_page() is an odd corner, which may one day give the "stable pages" DIF/DIX people trouble, though they've not yet complained.

I'd prefer a patch not specific to frontswap, but along the lines below:
I think that's the most robust way to express it, though I don't think the (count == 0) case can actually occur inside that block (whereas count == 0 certainly can occur in the !PageSwapCache case).

Quoted 4 lines

I believe that I once upon a time took statistics of how often the PageWriteback case happens here, and concluded that it wasn't often enough that refusing to reuse in this case would be likely to slow anyone down noticeably.

Quoted 5 lines

I agree. I had a test about that with zram and that case wasn't common. so your patch looks good to me.

I am waiting Dan's reply(He will come in this week) and then, judge what's the best.

Quoted 7 lines

Hugh is right that handling the possibility of duplicates is part of the tmem ABI. If there is any possibility of duplicates, the ABI defines how a backend must handle them to avoid data coherency issues.

The kernel implements an in-kernel API which implements the tmem ABI. If the frontend and backend can always agree that duplicate

Which ABI in zcache implement that?

Quoted 17 lines

are never possible, I agree that the backend could avoid that special case. However, duplicates occur rarely enough and the consequences (data loss) are bad enough that I think the case should still be checked, at least with a BUG_ON. I also wonder if it is worth it to make changes to the core swap subsystem to avoid code to implement a zswap corner case.

Remember that zswap is an oversimplified special case of tmem that handles only one frontend (Linux frontswap) and one backend (zswap). Tmem goes well beyond that and already supports other more general backends including Xen and ramster, and could also support other frontends such as a BSD or Solaris equivalent of frontswap, for example with a Linux ramster/zcache backend. I'm not sure how wise it is to tear out generic code and replace it with simplistic code unless there is absolutely no chance that the generic code will be necessary.

Quoted 8 lines

My two cents,
Dan

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

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

#6 lore raw
Quoted 39 lines

From: Ric Mason [mailto:[email protected]]
Sent: Tuesday, February 19, 2013 5:12 AM
To: Dan Magenheimer
Cc: Minchan Kim; Hugh Dickins; Nitin Gupta; Seth Jennings; Konrad Rzeszutek Wilk; [email protected]; [email protected]; Andrew Morton
Subject: Re: Questin about swap_slot free and invalidate page

Quoted 31 lines · Dan Magenheimer
Quoted 19 lines

From: Minchan Kim [mailto:[email protected]]
Sent: Sunday, February 03, 2013 7:50 PM
To: Hugh Dickins
Cc: Nitin Gupta; Dan Magenheimer; Seth Jennings; Konrad Rzeszutek Wilk; [email protected]; linux- [email protected]; Andrew Morton
Subject: Re: Questin about swap_slot free and invalidate page

Hi Hugh,

Quoted 9 lines · Hugh Dickins
Quoted 7 lines · Minchan Kim

When I reviewed zswap, I was curious about frontswap_store. It said following as.

* If frontswap already contains a page with matching swaptype and
* offset, the frontswap implementation may either overwrite the data and
* return success or invalidate the page from frontswap and return failure.

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines

It didn't say why it happens. we already have __frontswap_invalidate_page and call it whenever swap_slot frees. If we don't free swap slot, scan_swap_map can't find the slot for swap out so I thought overwriting of data shouldn't happen in frontswap.

I am waiting Dan's reply(He will come in this week) and then, judge what's the best.

Hugh is right that handling the possibility of duplicates is part of the tmem ABI. If there is any possibility of duplicates, the ABI defines how a backend must handle them to avoid data coherency issues.

Quoted 4 lines
Quoted 2 lines

The kernel implements an in-kernel API which implements the tmem ABI. If the frontend and backend can always agree that duplicate

Which ABI in zcache implement that?

https://oss.oracle.com/projects/tmem/dist/documentation/api/tmemspec-v001.pdf

The in-kernel APIs are frontswap and cleancache. For more information about tmem, see http://lwn.net/Articles/454795/

Quoted 7 lines
Quoted 7 lines

are never possible, I agree that the backend could avoid that special case. However, duplicates occur rarely enough and the consequences (data loss) are bad enough that I think the case should still be checked, at least with a BUG_ON. I also wonder if it is worth it to make changes to the core swap subsystem to avoid code to implement a zswap corner case.

Quoted 9 lines
Quoted 9 lines

Remember that zswap is an oversimplified special case of tmem that handles only one frontend (Linux frontswap) and one backend (zswap). Tmem goes well beyond that and already supports other more general backends including Xen and ramster, and could also support other frontends such as a BSD or Solaris equivalent of frontswap, for example with a Linux ramster/zcache backend. I'm not sure how wise it is to tear out generic code and replace it with simplistic code unless there is absolutely no chance that the generic code will be necessary.

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

#7 lore raw
Quoted 39 lines · Dan Magenheimer
Quoted 39 lines

From: Ric Mason [mailto:[email protected]]
Sent: Tuesday, February 19, 2013 5:12 AM
To: Dan Magenheimer
Cc: Minchan Kim; Hugh Dickins; Nitin Gupta; Seth Jennings; Konrad Rzeszutek Wilk; [email protected]; [email protected]; Andrew Morton
Subject: Re: Questin about swap_slot free and invalidate page

Quoted 31 lines · Dan Magenheimer
Quoted 19 lines

From: Minchan Kim [mailto:[email protected]]
Sent: Sunday, February 03, 2013 7:50 PM
To: Hugh Dickins
Cc: Nitin Gupta; Dan Magenheimer; Seth Jennings; Konrad Rzeszutek Wilk; [email protected]; linux- [email protected]; Andrew Morton
Subject: Re: Questin about swap_slot free and invalidate page

Hi Hugh,

Quoted 9 lines · Hugh Dickins
Quoted 7 lines · Minchan Kim

When I reviewed zswap, I was curious about frontswap_store. It said following as.

* If frontswap already contains a page with matching swaptype and
* offset, the frontswap implementation may either overwrite the data and
* return success or invalidate the page from frontswap and return failure.

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines

It didn't say why it happens. we already have __frontswap_invalidate_page and call it whenever swap_slot frees. If we don't free swap slot, scan_swap_map can't find the slot for swap out so I thought overwriting of data shouldn't happen in frontswap.

I am waiting Dan's reply(He will come in this week) and then, judge what's the best.

Hugh is right that handling the possibility of duplicates is part of the tmem ABI. If there is any possibility of duplicates, the ABI defines how a backend must handle them to avoid data coherency issues.

Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

The kernel implements an in-kernel API which implements the tmem ABI. If the frontend and backend can always agree that duplicate

Which ABI in zcache implement that?

https://oss.oracle.com/projects/tmem/dist/documentation/api/tmemspec-v001.pdf

The in-kernel APIs are frontswap and cleancache. For more information about tmem, see http://lwn.net/Articles/454795/

But you mentioned that you have in-kernel API which can handle duplicate. Do you mean zcache_cleancache/frontswap_put_page? I think they just overwrite instead of optional flush the page on the second(duplicate) put as mentioned in your tmemspec.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

are never possible, I agree that the backend could avoid that special case. However, duplicates occur rarely enough and the consequences (data loss) are bad enough that I think the case should still be checked, at least with a BUG_ON. I also wonder if it is worth it to make changes to the core swap subsystem to avoid code to implement a zswap corner case.

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

Remember that zswap is an oversimplified special case of tmem that handles only one frontend (Linux frontswap) and one backend (zswap). Tmem goes well beyond that and already supports other more general backends including Xen and ramster, and could also support other frontends such as a BSD or Solaris equivalent of frontswap, for example with a Linux ramster/zcache backend. I'm not sure how wise it is to tear out generic code and replace it with simplistic code unless there is absolutely no chance that the generic code will be necessary.

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

#8 lore raw
Quoted 15 lines

From: Ric Mason [mailto:[email protected]]
Subject: Re: Questin about swap_slot free and invalidate page

Quoted 11 lines · Dan Magenheimer
Quoted 9 lines

From: Ric Mason [mailto:[email protected]]

Quoted 7 lines

Hugh is right that handling the possibility of duplicates is part of the tmem ABI. If there is any possibility of duplicates, the ABI defines how a backend must handle them to avoid data coherency issues.

The kernel implements an in-kernel API which implements the tmem ABI. If the frontend and backend can always agree that duplicate

Which ABI in zcache implement that?

https://oss.oracle.com/projects/tmem/dist/documentation/api/tmemspec-v001.pdf

Quoted 7 lines
Quoted 2 lines

The in-kernel APIs are frontswap and cleancache. For more information about tmem, see http://lwn.net/Articles/454795/

But you mentioned that you have in-kernel API which can handle duplicate. Do you mean zcache_cleancache/frontswap_put_page? I think they just overwrite instead of optional flush the page on the second(duplicate) put as mentioned in your tmemspec.

Maybe I am misunderstanding your question... The spec allows overwrite (and return success) OR flush the page (and return failure). Zcache does the latter (flush). The code that implements it is in tmem_put.

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

#9 lore raw
Quoted 17 lines · Dan Magenheimer
Quoted 17 lines

From: Ric Mason [mailto:[email protected]]
Subject: Re: Questin about swap_slot free and invalidate page

Quoted 13 lines · Dan Magenheimer
Quoted 9 lines

From: Ric Mason [mailto:[email protected]]

Quoted 7 lines

Hugh is right that handling the possibility of duplicates is part of the tmem ABI. If there is any possibility of duplicates, the ABI defines how a backend must handle them to avoid data coherency issues.

The kernel implements an in-kernel API which implements the tmem ABI. If the frontend and backend can always agree that duplicate

Which ABI in zcache implement that?

https://oss.oracle.com/projects/tmem/dist/documentation/api/tmemspec-v001.pdf

The in-kernel APIs are frontswap and cleancache. For more information about tmem, see http://lwn.net/Articles/454795/

Quoted 8 lines
Quoted 4 lines

But you mentioned that you have in-kernel API which can handle duplicate. Do you mean zcache_cleancache/frontswap_put_page? I think they just overwrite instead of optional flush the page on the second(duplicate) put as mentioned in your tmemspec.

Maybe I am misunderstanding your question... The spec allows overwrite (and return success) OR flush the page (and return failure). Zcache does the latter (flush). The code that implements it is in tmem_put.

Thanks for your point out. Pers pages can have duplicate put since swap cache page can be reused. Can eph pages also have duplicate put? If yes, when can happen?

Quoted 1 line

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

#10 lore raw
Quoted 25 lines

From: Ric Mason [mailto:[email protected]]
Subject: Re: Questin about swap_slot free and invalidate page

Quoted 21 lines · Dan Magenheimer
Quoted 21 lines

From: Ric Mason [mailto:[email protected]]
Subject: Re: Questin about swap_slot free and invalidate page

Quoted 6 lines · Dan Magenheimer
Quoted 6 lines

From: Ric Mason [mailto:[email protected]]

Quoted 5 lines

Hugh is right that handling the possibility of duplicates is part of the tmem ABI. If there is any possibility of duplicates, the ABI defines how a backend must handle them to avoid data coherency issues.

Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

The kernel implements an in-kernel API which implements the tmem ABI. If the frontend and backend can always agree that duplicate

Which ABI in zcache implement that?

https://oss.oracle.com/projects/tmem/dist/documentation/api/tmemspec-v001.pdf

The in-kernel APIs are frontswap and cleancache. For more information about tmem, see http://lwn.net/Articles/454795/

But you mentioned that you have in-kernel API which can handle duplicate. Do you mean zcache_cleancache/frontswap_put_page? I think they just overwrite instead of optional flush the page on the second(duplicate) put as mentioned in your tmemspec.

Quoted 8 lines
Quoted 4 lines

Maybe I am misunderstanding your question... The spec allows overwrite (and return success) OR flush the page (and return failure). Zcache does the latter (flush). The code that implements it is in tmem_put.

Thanks for your point out. Pers pages can have duplicate put since swap cache page can be reused. Can eph pages also have duplicate put? If yes, when can happen?

Yes, I have seen it. I am not sure of the exact circumstances when it happens as I am not an expert in the VFS subsystem. (Chris Mason wrote the VFS cleancache hooks in 2009.)

Dan

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