Re: [PATCH v4 2/2] mm/gup/writeback: add callbacks for inaccessible pages

[Dave Hansen <dave.hansen@intel.com>]

On 3/6/20 5:25 AM, Claudio Imbrenda wrote:
> On s390x the function is not supposed to fail, so it is ok to use a
> WARN_ON on failure. If we ever need some more finegrained handling
> we can tackle this when we know the details.

Could you explain a bit why the function can't fail?

If the guest has secret data in the page, then it *can* and does fail.
It won't fail, though, if the host and guest agree on whether the page
is protected.

Right?

> @@ -2807,6 +2807,13 @@ int __test_set_page_writeback(struct page *page, bool keep_write)
>  		inc_zone_page_state(page, NR_ZONE_WRITE_PENDING);
>  	}
>  	unlock_page_memcg(page);
> +	access_ret = arch_make_page_accessible(page);
> +	/*
> +	 * If writeback has been triggered on a page that cannot be made
> +	 * accessible, it is too late to recover here.
> +	 */
> +	VM_BUG_ON_PAGE(access_ret != 0, page);
> +
>  	return ret;
>  
>  }

This seems like a really odd place to do this.  Writeback is specific to
block I/O.  I would have thought there were other kinds of devices that
matter, not just block devices.

Also, this patch seems odd that it only does the
arch_make_page_accessible() half.  Where's the other half where the page
is made inaccessible?

I assume it's OK to "leak" things like this, it's just not clear to me
_why_ it's OK.