Hi John, Thank you for looking at this series.
static inline void set_page_refcounted(struct page *page) { + int refcnt; + VM_BUG_ON_PAGE(PageTail(page), page); VM_BUG_ON_PAGE(page_ref_count(page), page); - set_page_count(page, 1); + refcnt = page_ref_inc_return(page); + VM_BUG_ON_PAGE(refcnt != 1, page);
I am acutely uncomfortable with this change, because it changes the meaning and behavior of the function to something completely different, while leaving the function name unchanged. Furthermore, in relies upon debug assertions, rather than a return value (for example) to verify that all is well.
It must return the same thing, if it does not we have a bug in our kernel which may lead to memory corruptions and security holes. So today we have this: VM_BUG_ON_PAGE(page_ref_count(page), page); -> check ref_count is 0 < What if something modified here? Hmm..> set_page_count(page, 1); -> Yet we reset it to 1. With my proposed change: VM_BUG_ON_PAGE(page_ref_count(page), page); -> check ref_count is 0 refcnt = page_ref_inc_return(page); -> ref_count better be 1. VM_BUG_ON_PAGE(refcnt != 1, page); -> Verify that it is 1.
I understand where this patchset is going, but this intermediate step is not a good move. Also, for the overall series, if you want to change from "set_page_count()" to "inc_and_verify_val_equals_one()", then the way to do that is *not* to depend solely on VM_BUG*() to verify. Instead, return something like -EBUSY if incrementing the value results in a surprise, and let the caller decide how to handle it.
Actually, -EBUSY would be OK if the problems were because we failed to modify refcount for some reason, but if we modified refcount and got an unexpected value (i.e underflow/overflow) we better report it right away instead of waiting for memory corruption to happen. Thanks, Pasha