| Message ID | 20240126132903.2700077-8-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/21] mm: move mapping_set_update out of <linux/swap.h> | expand |
On Fri, Jan 26, 2024 at 02:28:49PM +0100, Christoph Hellwig wrote: > +++ b/mm/shmem.c > @@ -2150,6 +2150,11 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, > * > * Return: The found folio, %NULL if SGP_READ or SGP_NOALLOC was passed in @sgp > * and no folio was found at @index, or an ERR_PTR() otherwise. > + * > + * If the caller modifies data in the returned folio, it must call > + * folio_mark_dirty() on the locked folio before dropping the reference to > + * ensure the folio is not reclaimed. Unlike for normal file systems there is > + * no need to reserve space for users of shmem_*file_setup(). This doesn't quite make sense to me. Do you mean: * If the caller modifies data in the folio, it must call folio_mark_dirty() * before unlocking the folio to ensure that the folio is not reclaimed. * There is no equivalent to write_begin/write_end for shmem. (also this should go before the Return: section; the return section should be the last thing in the kernel-doc)
On Fri, Jan 26, 2024 at 03:49:30PM +0000, Matthew Wilcox wrote: > This doesn't quite make sense to me. Do you mean: > > * If the caller modifies data in the folio, it must call folio_mark_dirty() > * before unlocking the folio to ensure that the folio is not reclaimed. > * There is no equivalent to write_begin/write_end for shmem. > > (also this should go before the Return: section; the return section > should be the last thing in the kernel-doc) So the first sentence and moving the section makes total sense. The second sentence I don't think is very useful. write_begin/write_end are relaly just a way for generic_perform_write to do the space reservation and extending i_size and not really methods in the classic sense. They should go away from a_ops and certainly don't end up being mentioned in shmem.c. What I have now is this: If the caller modifies data in the folio, it must call folio_mark_dirty() before unlocking the folio to ensure that the folio is not reclaimed. These is no need to reserve space before calling folio_mark_dirty(). but I'm open to further improvements.
On Sun, Jan 28, 2024 at 05:54:34PM +0100, Christoph Hellwig wrote: > On Fri, Jan 26, 2024 at 03:49:30PM +0000, Matthew Wilcox wrote: > > This doesn't quite make sense to me. Do you mean: > > > > * If the caller modifies data in the folio, it must call folio_mark_dirty() > > * before unlocking the folio to ensure that the folio is not reclaimed. > > * There is no equivalent to write_begin/write_end for shmem. > > > > (also this should go before the Return: section; the return section > > should be the last thing in the kernel-doc) > > So the first sentence and moving the section makes total sense. > The second sentence I don't think is very useful. write_begin/write_end > are relaly just a way for generic_perform_write to do the space > reservation and extending i_size and not really methods in the classic > sense. They should go away from a_ops and certainly don't end up > being mentioned in shmem.c. > > What I have now is this: > > If the caller modifies data in the folio, it must call folio_mark_dirty() > before unlocking the folio to ensure that the folio is not reclaimed. > These is no need to reserve space before calling folio_mark_dirty(). That's totally fine with me. Could I trouble you to elaborate on what you'd like to see a filesystem like ubifs do to replace write_begin/write_end? After my recent patches, those are the only places in ubifs that have a struct page reference. I've been holding off on converting those and writepage because we have plans to eliminate them, but I'm not sure how much longer we can hold off.
On Mon, Jan 29, 2024 at 03:56:58PM +0000, Matthew Wilcox wrote: > Could I trouble you to elaborate on what you'd like to see a filesystem > like ubifs do to replace write_begin/write_end? After my recent patches, > those are the only places in ubifs that have a struct page reference. > I've been holding off on converting those and writepage because we have > plans to eliminate them, but I'm not sure how much longer we can hold off. I think the abstraction is okay for anyone using generic_perform_write. The problem with them is that they are not generic operations called by higher level code, but callbacks that depend on caller context. So in perfect world they would be passed directly to generic_perform_write and the few other users and not go through aops.
diff --git a/mm/shmem.c b/mm/shmem.c index e89fb5eccb0c0a..f7d6848fb294d6 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2150,6 +2150,11 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, * * Return: The found folio, %NULL if SGP_READ or SGP_NOALLOC was passed in @sgp * and no folio was found at @index, or an ERR_PTR() otherwise. + * + * If the caller modifies data in the returned folio, it must call + * folio_mark_dirty() on the locked folio before dropping the reference to + * ensure the folio is not reclaimed. Unlike for normal file systems there is + * no need to reserve space for users of shmem_*file_setup(). */ int shmem_get_folio(struct inode *inode, pgoff_t index, struct folio **foliop, enum sgp_type sgp)