| Message ID | 20240704163724.2462161-2-martin.oliveira@eideticom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable P2PDMA in Userspace RDMA | expand |
On Thu, Jul 04, 2024 at 10:37:22AM -0600, Martin Oliveira wrote: > The .page_mkwrite operator of kernfs just calls file_update_time(). > This is the same behaviour that the fault code does if .page_mkwrite is > not set. > > Furthermore, having the page_mkwrite() operator causes > writable_file_mapping_allowed() to fail due to > vma_needs_dirty_tracking() on the gup flow, which is a pre-requisite for > enabling P2PDMA over RDMA. > > There are no users of .page_mkwrite and no known valid use cases, so > just remove the .page_mkwrite from kernfs_ops and WARN_ON() if an mmap() > implementation sets .page_mkwrite. > > Co-developed-by: Logan Gunthorpe <logang@deltatee.com> > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > Signed-off-by: Martin Oliveira <martin.oliveira@eideticom.com> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Thu, Jul 04, 2024 at 10:37:22AM -0600, Martin Oliveira wrote: > @@ -482,6 +459,8 @@ static int kernfs_fop_mmap(struct file *file, struct vm_area_struct *vma) > if (vma->vm_ops && vma->vm_ops->close) > goto out_put; > > + WARN_ON(vma->vm_ops && vma->vm_ops->page_mkwrite); > + > rc = 0; > if (!of->mmapped) { > of->mmapped = true; Seems to me we should actually _handle_ that, not do something wrong. eg: if (vma->vm_ops) { if (vma->vm_ops->close) goto out_put; if (WARN_ON(vma->vm_ops->page_mkwrite)) goto out_put; } or maybe this doesn't need to be a WARN at all? After all, there isn't one for having a ->close method, so why is page_mkwrite special?
On 2024-07-04 11:02, Matthew Wilcox wrote:> Seems to me we should actually _handle_ that, not do something wrong. > eg: > > if (vma->vm_ops) { > if (vma->vm_ops->close) > goto out_put; > if (WARN_ON(vma->vm_ops->page_mkwrite)) > goto out_put; > } Good point. > or maybe this doesn't need to be a WARN at all? After all, there > isn't one for having a ->close method, so why is page_mkwrite special? Hmm yeah, they should probably be treated the same. Maybe ->close should be converted to WARN as well? It would be easier to catch an error this way than chasing the EINVAL, but I'm OK either way. Thanks, Martin
On Thu, Jul 04, 2024 at 02:43:04PM -0600, Martin Oliveira wrote: > On 2024-07-04 11:02, Matthew Wilcox wrote:> Seems to me we should actually _handle_ that, not do something wrong. > > eg: > > > > if (vma->vm_ops) { > > if (vma->vm_ops->close) > > goto out_put; > > if (WARN_ON(vma->vm_ops->page_mkwrite)) > > goto out_put; > > } > > Good point. Btw, sorry if I mislead you with my WARN_ON_ONCE suggestion. That was always intended in addition to the error handling, not instead. (In fact there are very few reasons to use WARN_ON* without actually handling the error as well). > > > or maybe this doesn't need to be a WARN at all? After all, there > > isn't one for having a ->close method, so why is page_mkwrite special? > > Hmm yeah, they should probably be treated the same. > > Maybe ->close should be converted to WARN as well? It would be easier to > catch an error this way than chasing the EINVAL, but I'm OK either way. Yes, doing the same for ->close or anything unimplemented would be nice. But it's not really in scope for this series. kernfs really should be using it's own ops instead of abusing file_operations, but that's even more out of scope..
On 2024-07-05 01:20, Christoph Hellwig wrote: > Btw, sorry if I mislead you with my WARN_ON_ONCE suggestion. That > was always intended in addition to the error handling, not instead. > (In fact there are very few reasons to use WARN_ON* without actually > handling the error as well). Yeah, I should have caught that. Thanks for the feedback, Christoph! I'll submit a new version later today. > Yes, doing the same for ->close or anything unimplemented would be > nice. But it's not really in scope for this series. > > kernfs really should be using it's own ops instead of abusing > file_operations, but that's even more out of scope.. Ok, I'll add the ->page_mkwrite with the WARN but leave the ->close the way it is. Martin
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c index 8502ef68459b..90603664de7f 100644 --- a/fs/kernfs/file.c +++ b/fs/kernfs/file.c @@ -386,28 +386,6 @@ static vm_fault_t kernfs_vma_fault(struct vm_fault *vmf) return ret; } -static vm_fault_t kernfs_vma_page_mkwrite(struct vm_fault *vmf) -{ - struct file *file = vmf->vma->vm_file; - struct kernfs_open_file *of = kernfs_of(file); - vm_fault_t ret; - - if (!of->vm_ops) - return VM_FAULT_SIGBUS; - - if (!kernfs_get_active(of->kn)) - return VM_FAULT_SIGBUS; - - ret = 0; - if (of->vm_ops->page_mkwrite) - ret = of->vm_ops->page_mkwrite(vmf); - else - file_update_time(file); - - kernfs_put_active(of->kn); - return ret; -} - static int kernfs_vma_access(struct vm_area_struct *vma, unsigned long addr, void *buf, int len, int write) { @@ -432,7 +410,6 @@ static int kernfs_vma_access(struct vm_area_struct *vma, unsigned long addr, static const struct vm_operations_struct kernfs_vm_ops = { .open = kernfs_vma_open, .fault = kernfs_vma_fault, - .page_mkwrite = kernfs_vma_page_mkwrite, .access = kernfs_vma_access, }; @@ -482,6 +459,8 @@ static int kernfs_fop_mmap(struct file *file, struct vm_area_struct *vma) if (vma->vm_ops && vma->vm_ops->close) goto out_put; + WARN_ON(vma->vm_ops && vma->vm_ops->page_mkwrite); + rc = 0; if (!of->mmapped) { of->mmapped = true;