On Wed, May 12, 2021 at 12:10:16AM +0900, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
>
> Now __get_hwpoison_page() could return -EBUSY in a race condition,
> so it's helpful to handle it by retrying. We already have retry
> logic, so make get_hwpoison_page() call get_any_page() when called
> from memory_failure().
As I stated in your previous patch, I think you should start returning -EBUSY
from this patch onwards.
> static int get_any_page(struct page *p, unsigned long flags)
> {
> int ret = 0, pass = 0;
> @@ -1152,50 +1136,76 @@ static int get_any_page(struct page *p, unsigned long flags)
> count_increased = true;
>
> try_again:
> - if (!count_increased && !__get_hwpoison_page(p)) {
> - if (page_count(p)) {
> - /* We raced with an allocation, retry. */
> - if (pass++ < 3)
> - goto try_again;
> - ret = -EBUSY;
> - } else if (!PageHuge(p) && !is_free_buddy_page(p)) {
> - /* We raced with put_page, retry. */
> - if (pass++ < 3)
> - goto try_again;
> - ret = -EIO;
> + if (!count_increased) {
> + ret = __get_hwpoison_page(p);
> + if (!ret) {
> + if (page_count(p)) {
> + /* We raced with an allocation, retry. */
> + if (pass++ < 3)
> + goto try_again;
> + ret = -EBUSY;
> + } else if (!PageHuge(p) && !is_free_buddy_page(p)) {
> + /* We raced with put_page, retry. */
> + if (pass++ < 3)
> + goto try_again;
> + ret = -EIO;
> + }
> + goto out;
> }
> + }
I think this can be improved.
We cannot have -EBUSY unless we come from __get_hwpoison_page() (!count_increased),
so I think a much more natural approach would be to stuff the hunk below in there,
and then place the other stuff in an else, so something like:
if (!count_increased) {
ret = __get_hwpoison_page(p);
if (!ret) {
if (page_count(p)) {
/* We raced with an allocation, retry. */
if (pass++ < 3)
goto try_again;
ret = -EBUSY;
} else if (!PageHuge(p) && !is_free_buddy_page(p)) {
/* We raced with put_page, retry. */
if (pass++ < 3)
goto try_again;
ret = -EIO;
}
goto out;
} else if (ret == -EBUSY) {
/* We raced with freeing huge page to buddy, retry. */
if (pass++ < 3)
goto try_again;
}
} else {
/* We do already have a refcount for this page, see if we can
* handle it.
*/
if (PageHuge(p) || PageLRU(p) || __PageMovable(p)) {
ret = 1;
} else {
/* A page we cannot handle. Check...
}
}
Other than that, looks good to me.
> +
> + if (ret == -EBUSY) {
> + /* We raced with freeing huge page to buddy, retry. */
> + if (pass++ < 3)
> + goto try_again;
--
Oscar Salvador
SUSE L3
