| Message ID | 20231107212643.3490372-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Add folio_zero_tail() and folio_fill_tail() | expand |
On Tue, Nov 7, 2023 at 10:27 PM Matthew Wilcox (Oracle) <willy@infradead.org> wrote: > The iomap code was limited to PAGE_SIZE bytes; generalise it to cover > an arbitrary-sized folio, and move it to be a common helper. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/iomap/buffered-io.c | 14 ++------------ > include/linux/highmem.h | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 40 insertions(+), 12 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index f72df2babe56..093c4515b22a 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -305,28 +305,18 @@ static int iomap_read_inline_data(const struct iomap_iter *iter, > { > const struct iomap *iomap = iomap_iter_srcmap(iter); > size_t size = i_size_read(iter->inode) - iomap->offset; > - size_t poff = offset_in_page(iomap->offset); > size_t offset = offset_in_folio(folio, iomap->offset); > - void *addr; > > if (folio_test_uptodate(folio)) > return 0; > > - if (WARN_ON_ONCE(size > PAGE_SIZE - poff)) > - return -EIO; > - if (WARN_ON_ONCE(size > PAGE_SIZE - > - offset_in_page(iomap->inline_data))) > - return -EIO; > if (WARN_ON_ONCE(size > iomap->length)) > return -EIO; > if (offset > 0) > ifs_alloc(iter->inode, folio, iter->flags); > > - addr = kmap_local_folio(folio, offset); > - memcpy(addr, iomap->inline_data, size); > - memset(addr + size, 0, PAGE_SIZE - poff - size); > - kunmap_local(addr); > - iomap_set_range_uptodate(folio, offset, PAGE_SIZE - poff); > + folio_fill_tail(folio, offset, iomap->inline_data, size); > + iomap_set_range_uptodate(folio, offset, folio_size(folio) - offset); > return 0; > } > > diff --git a/include/linux/highmem.h b/include/linux/highmem.h > index 1b81416196dd..0fbb60ffefc9 100644 > --- a/include/linux/highmem.h > +++ b/include/linux/highmem.h > @@ -521,6 +521,44 @@ static inline __must_check void *folio_zero_tail(struct folio *folio, > return kaddr; > } > > +/** > + * folio_fill_tail - Copy some data to a folio and pad with zeroes. > + * @folio: The destination folio. > + * @offset: The offset into @folio at which to start copying. > + * @from: The data to copy. > + * @len: How many bytes of data to copy. > + * > + * This function is most useful for filesystems which support inline data. > + * When they want to copy data from the inode into the page cache, this > + * function does everything for them. It supports large folios even on > + * HIGHMEM configurations. > + */ > +static inline void folio_fill_tail(struct folio *folio, size_t offset, > + const char *from, size_t len) > +{ > + char *to = kmap_local_folio(folio, offset); > + > + VM_BUG_ON(offset + len > folio_size(folio)); > + > + if (folio_test_highmem(folio)) { > + size_t max = PAGE_SIZE - offset_in_page(offset); > + > + while (len > max) { > + memcpy(to, from, max); > + kunmap_local(to); > + len -= max; > + from += max; > + offset += max; > + max = PAGE_SIZE; > + to = kmap_local_folio(folio, offset); > + } > + } > + > + memcpy(to, from, len); > + to = folio_zero_tail(folio, offset, to); This needs to be: to = folio_zero_tail(folio, offset + len, to + len); > + kunmap_local(to); > +} > + > /** > * memcpy_from_file_folio - Copy some bytes from a file folio. > * @to: The destination buffer. > -- > 2.42.0 > Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com> Thanks, Andreas
On Thu, Nov 09, 2023 at 10:50:45PM +0100, Andreas Gruenbacher wrote: > On Tue, Nov 7, 2023 at 10:27 PM Matthew Wilcox (Oracle) > <willy@infradead.org> wrote: > > +static inline void folio_fill_tail(struct folio *folio, size_t offset, > > + const char *from, size_t len) > > +{ > > + char *to = kmap_local_folio(folio, offset); > > + > > + VM_BUG_ON(offset + len > folio_size(folio)); > > + > > + if (folio_test_highmem(folio)) { > > + size_t max = PAGE_SIZE - offset_in_page(offset); > > + > > + while (len > max) { > > + memcpy(to, from, max); > > + kunmap_local(to); > > + len -= max; > > + from += max; > > + offset += max; > > + max = PAGE_SIZE; > > + to = kmap_local_folio(folio, offset); > > + } > > + } > > + > > + memcpy(to, from, len); > > + to = folio_zero_tail(folio, offset, to); > > This needs to be: > > to = folio_zero_tail(folio, offset + len, to + len); Oh, wow, that was stupid of me. I only ran an xfstests against ext4, which doesn't exercise this code, not gfs2 or erofs. Thanks for fixing this up. I was wondering about adding the assertion: VM_BUG_ON((kaddr - offset) % PAGE_SIZE); to catch the possible mistake of calling kmap_local_folio(folio, 0) instead of kmap_local_folio(folio, offset). But maybe that's sufficiently unlikely a mistake to bother adding a runtime check for.
Am Fr., 10. Nov. 2023 um 18:09 Uhr schrieb Matthew Wilcox <willy@infradead.org>: > On Thu, Nov 09, 2023 at 10:50:45PM +0100, Andreas Gruenbacher wrote: > > On Tue, Nov 7, 2023 at 10:27 PM Matthew Wilcox (Oracle) > > <willy@infradead.org> wrote: > > > +static inline void folio_fill_tail(struct folio *folio, size_t offset, > > > + const char *from, size_t len) > > > +{ > > > + char *to = kmap_local_folio(folio, offset); > > > + > > > + VM_BUG_ON(offset + len > folio_size(folio)); > > > + > > > + if (folio_test_highmem(folio)) { > > > + size_t max = PAGE_SIZE - offset_in_page(offset); > > > + > > > + while (len > max) { > > > + memcpy(to, from, max); > > > + kunmap_local(to); > > > + len -= max; > > > + from += max; > > > + offset += max; > > > + max = PAGE_SIZE; > > > + to = kmap_local_folio(folio, offset); > > > + } > > > + } > > > + > > > + memcpy(to, from, len); > > > + to = folio_zero_tail(folio, offset, to); > > > > This needs to be: > > > > to = folio_zero_tail(folio, offset + len, to + len); > > Oh, wow, that was stupid of me. I only ran an xfstests against ext4, > which doesn't exercise this code, not gfs2 or erofs. Thanks for > fixing this up. > > I was wondering about adding the assertion: > > VM_BUG_ON((kaddr - offset) % PAGE_SIZE); > > to catch the possible mistake of calling kmap_local_folio(folio, 0) > instead of kmap_local_folio(folio, offset). But maybe that's > sufficiently unlikely a mistake to bother adding a runtime check for. folio_zero_tail() is a bit of an obscure function, so I'm not sure if there will be additional callers. The parameters are described as: * @offset: The byte offset in the folio to start zeroing at. * @kaddr: The address the folio is currently mapped to. What about changing the @kaddr description to 'the (mapped) address within the folio to start zeroing at' or similar? Andreas
On 2023/11/11 01:09, Matthew Wilcox wrote: > On Thu, Nov 09, 2023 at 10:50:45PM +0100, Andreas Gruenbacher wrote: >> On Tue, Nov 7, 2023 at 10:27 PM Matthew Wilcox (Oracle) >> <willy@infradead.org> wrote: >>> +static inline void folio_fill_tail(struct folio *folio, size_t offset, >>> + const char *from, size_t len) >>> +{ >>> + char *to = kmap_local_folio(folio, offset); >>> + >>> + VM_BUG_ON(offset + len > folio_size(folio)); >>> + >>> + if (folio_test_highmem(folio)) { >>> + size_t max = PAGE_SIZE - offset_in_page(offset); >>> + >>> + while (len > max) { >>> + memcpy(to, from, max); >>> + kunmap_local(to); >>> + len -= max; >>> + from += max; >>> + offset += max; >>> + max = PAGE_SIZE; >>> + to = kmap_local_folio(folio, offset); >>> + } >>> + } >>> + >>> + memcpy(to, from, len); >>> + to = folio_zero_tail(folio, offset, to); >> >> This needs to be: >> >> to = folio_zero_tail(folio, offset + len, to + len); > > Oh, wow, that was stupid of me. I only ran an xfstests against ext4, > which doesn't exercise this code, not gfs2 or erofs. Thanks for > fixing this up. Assuming that is for the next cycle (no rush), I will also test this patch and feedback later since I'm now working on other stuffs. Thanks, Gao Xiang
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index f72df2babe56..093c4515b22a 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -305,28 +305,18 @@ static int iomap_read_inline_data(const struct iomap_iter *iter, { const struct iomap *iomap = iomap_iter_srcmap(iter); size_t size = i_size_read(iter->inode) - iomap->offset; - size_t poff = offset_in_page(iomap->offset); size_t offset = offset_in_folio(folio, iomap->offset); - void *addr; if (folio_test_uptodate(folio)) return 0; - if (WARN_ON_ONCE(size > PAGE_SIZE - poff)) - return -EIO; - if (WARN_ON_ONCE(size > PAGE_SIZE - - offset_in_page(iomap->inline_data))) - return -EIO; if (WARN_ON_ONCE(size > iomap->length)) return -EIO; if (offset > 0) ifs_alloc(iter->inode, folio, iter->flags); - addr = kmap_local_folio(folio, offset); - memcpy(addr, iomap->inline_data, size); - memset(addr + size, 0, PAGE_SIZE - poff - size); - kunmap_local(addr); - iomap_set_range_uptodate(folio, offset, PAGE_SIZE - poff); + folio_fill_tail(folio, offset, iomap->inline_data, size); + iomap_set_range_uptodate(folio, offset, folio_size(folio) - offset); return 0; } diff --git a/include/linux/highmem.h b/include/linux/highmem.h index 1b81416196dd..0fbb60ffefc9 100644 --- a/include/linux/highmem.h +++ b/include/linux/highmem.h @@ -521,6 +521,44 @@ static inline __must_check void *folio_zero_tail(struct folio *folio, return kaddr; } +/** + * folio_fill_tail - Copy some data to a folio and pad with zeroes. + * @folio: The destination folio. + * @offset: The offset into @folio at which to start copying. + * @from: The data to copy. + * @len: How many bytes of data to copy. + * + * This function is most useful for filesystems which support inline data. + * When they want to copy data from the inode into the page cache, this + * function does everything for them. It supports large folios even on + * HIGHMEM configurations. + */ +static inline void folio_fill_tail(struct folio *folio, size_t offset, + const char *from, size_t len) +{ + char *to = kmap_local_folio(folio, offset); + + VM_BUG_ON(offset + len > folio_size(folio)); + + if (folio_test_highmem(folio)) { + size_t max = PAGE_SIZE - offset_in_page(offset); + + while (len > max) { + memcpy(to, from, max); + kunmap_local(to); + len -= max; + from += max; + offset += max; + max = PAGE_SIZE; + to = kmap_local_folio(folio, offset); + } + } + + memcpy(to, from, len); + to = folio_zero_tail(folio, offset, to); + kunmap_local(to); +} + /** * memcpy_from_file_folio - Copy some bytes from a file folio. * @to: The destination buffer.
The iomap code was limited to PAGE_SIZE bytes; generalise it to cover an arbitrary-sized folio, and move it to be a common helper. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/iomap/buffered-io.c | 14 ++------------ include/linux/highmem.h | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 12 deletions(-)