On Thu, Sep 06, 2018 at 05:56:31PM +0530, Souptick Joarder wrote: > > Yes, I'd start with converting ext4_page_mkwrite() - that should be pretty > > straightforward - and we can leave block_page_mkwrite() as is for now. I > > don't think allocating other VM_FAULT_ codes is going to cut it as > > generally the filesystem may need to communicate different error codes back > > and you don't know in advance which are interesting. Changing the return values of ext4_page_mkwrite() and ext4_filemap_fault() is definitely safe. If you want to start changing the type of "ret" to vm_fault_t and introduce a new variable "err", now you have to be super careful not to screw things up. (I believe one of the earlier patches didn't get that right.) > As block_page_mkwrite() is getting called from 2 places in ext4 and nilfs and > both places fault handler code convert errno to VM_FAULT_CODE using > block_page_mkwrite_return(), is it required to migrate block_page_mkwrite() > to use vm_fault_t return type and further complicate the API or better to > leave this API in current state ?? So I don't see the point of changing return value block_page_mkwrite() (although to be honest I haven't see the value of the vm_fault_t change at all in the first place, at least not compared to the pain it has caused) but no, I don't think it's worth it. The API for block_page_mkwrite() can simply be defined as "0 on success, < 0 on error". You can add documentation that it's up to caller of block_page_mkwrite() to call block_page_mkwrite_return() to translate the error to a vm_fault_t. > > One solution for passing error codes we could use with vm_fault_t is a > > scheme similar to ERR_PTR. So we can store full error code in vm_fault_t > > and still have a plenty of space for the special VM_FAULT_ return codes... > > With that scheme converting block_page_mkwrite() would be trivial. > > > I didn't get this part. Any reference code will be helpful ? So what we do for functions that need to either return an error or a pointer is to call encode the error as a "pointer" by using ERR_PTR(), and the caller can determine whether or not it is a valid pointer or an error code by using IS_ERR_VALUE() and turning it back into an error by using PTR_ERR(). See include/linux/err.h. Similarly, all valid vm_fault_t's composed of VM_FAULT_xxx are positive integers, and all errors are passed using the kernel's convention of using a negative error code. So going through lots of machinations to return both an error code and a vm_fault_t *really* wasn't necessary. The issue, as near as I can understand things, for why we're going through all of this churn, was there was a concern that in the mm code, that all of the places which received a vm_fault_t would sometimes see a negative error code. The proposal here is to just *accept* that this will happen, and just simply have them *check* to see if it's a negative error code, and convert it to the appropriate vm_fault_t in that case. It puts the onus of the change on the mm layer, where as the "blast radius" of the vm_fault_t "cleanup" is spread out across a large number of subsystems. Which I wouldn't mind, if it wasn't causing pain. But it *is* causing pain. And it's common kernel convention to overload an error and a pointer using the exact same trick. We do it *all* over the place, and quite frankly, it's less error prone than changing functions to return a pointer and an error. No one has said, "let's do to the ERR_PTR convention what we've done to the vm_fault_t -- it's too confusing that a pointer might be an error, since people might forget to check for it." If they did that, it would be NACK'ed right, left and center. But yet it's a good idea for vm_fault_t? - Ted
