Re: [PATCH 1/6] kernfs: create vm_operations_struct without page_mkwrite()

[Logan Gunthorpe <logang@deltatee.com>]

Hi Greg,

On 2024-06-06 14:54, Greg Kroah-Hartman wrote:
> On Wed, Jun 05, 2024 at 01:29:29PM -0600, Martin Oliveira wrote:
>> The standard kernfs vm_ops installs a page_mkwrite() operator which
>> modifies the file update time on write.
>>
>> This not always required (or makes sense), such as in the P2PDMA, which
>> uses the sysfs file as an allocator from userspace.
> 
> That's not a good idea, please don't do that.  sysfs binary files are
> "pass through", why would you want to use this as an allocator?

The P2PDMA code already creates a binary attribute which is used to
allocate P2PDMA memory into userspace[1]. It was done this way a couple
of years ago at the suggestion of Christoph[2]. Using a sysfs attribute
made the code substantially simpler and got rid of a bunch of pseudofs
mess that was required when mmaping a char device. The attribute already
exists and is used by userspace so it's not something we can change at
this point.

The attribute has worked well for what was needed until we wanted to use
P2PDMA memory with FOLL_LONGTERM and GUP. That path specifically denies
FOLL_LONGTERM pins when the underlying VMA has a .page_mkwrite operator,
which sysfs/kernfs forces on us. P2PDMA doesn't benefit from this
operator in any way so the simplest thing is to remove it for this use case.

>> 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.
>>
>> Fix this by adding a new boolean on kernfs_ops to differentiate between
>> the different behaviours.
> 
> This isn't going to work well.

What about it are you worried won't work well? We're open to other
suggestions.

Thanks,

Logan

[1] https://elixir.bootlin.com/linux/latest/source/drivers/pci/p2pdma.c#L164
[2] https://lore.kernel.org/all/20220705075108.GB17451@lst.de/