| Message ID | 20240812121159.3775074-6-yi.zhang@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iomap: some minor non-critical fixes and improvements when block size < folio size | expand |
On Mon, Aug 12, 2024 at 08:11:58PM +0800, Zhang Yi wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > In __iomap_write_begin(), if we unaligned buffered write data to a hole > of a regular file, we only zero out the place where aligned to block > size that we don't want to write, but mark the whole range uptodate if > block size < folio size. This is wrong since the not zeroed part will > contains stale data and can be accessed by a concurrent buffered read > easily (on the filesystem may not hold inode->i_rwsem) once we mark the > range uptodate. Fix this by drop iomap_set_range_uptodate() in the > zeroing out branch. > > Fixes: 9dc55f1389f9 ("iomap: add support for sub-pagesize buffered I/O without buffer heads") > Reported-by: Matthew Wilcox <willy@infradead.org> > Closes: https://lore.kernel.org/all/ZqsN5ouQTEc1KAzV@casper.infradead.org/ > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > --- > fs/iomap/buffered-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index ac762de9a27f..96600405dbb5 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -744,8 +744,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, > poff, plen, srcmap); > if (status) > return status; > + iomap_set_range_uptodate(folio, poff, plen); > } > - iomap_set_range_uptodate(folio, poff, plen); Don't we need to iomap_set_range_uptodate for the bytes that we zeroed with folio_zero_segments? --D > } while ((block_start += plen) < block_end); > > return 0; > -- > 2.39.2 > >
On 2024/8/13 0:49, Darrick J. Wong wrote: > On Mon, Aug 12, 2024 at 08:11:58PM +0800, Zhang Yi wrote: >> From: Zhang Yi <yi.zhang@huawei.com> >> >> In __iomap_write_begin(), if we unaligned buffered write data to a hole >> of a regular file, we only zero out the place where aligned to block >> size that we don't want to write, but mark the whole range uptodate if >> block size < folio size. This is wrong since the not zeroed part will >> contains stale data and can be accessed by a concurrent buffered read >> easily (on the filesystem may not hold inode->i_rwsem) once we mark the >> range uptodate. Fix this by drop iomap_set_range_uptodate() in the >> zeroing out branch. >> >> Fixes: 9dc55f1389f9 ("iomap: add support for sub-pagesize buffered I/O without buffer heads") >> Reported-by: Matthew Wilcox <willy@infradead.org> >> Closes: https://lore.kernel.org/all/ZqsN5ouQTEc1KAzV@casper.infradead.org/ >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> >> --- >> fs/iomap/buffered-io.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c >> index ac762de9a27f..96600405dbb5 100644 >> --- a/fs/iomap/buffered-io.c >> +++ b/fs/iomap/buffered-io.c >> @@ -744,8 +744,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, >> poff, plen, srcmap); >> if (status) >> return status; >> + iomap_set_range_uptodate(folio, poff, plen); >> } >> - iomap_set_range_uptodate(folio, poff, plen); > > Don't we need to iomap_set_range_uptodate for the bytes that we zeroed > with folio_zero_segments? > We must do partial block zeroing here, hence we don't need to set update bit. Thanks, Yi. > --D > >> } while ((block_start += plen) < block_end); >> >> return 0; >> -- >> 2.39.2 >> >>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, Aug 12, 2024 at 08:11:58PM +0800, Zhang Yi wrote: > +++ b/fs/iomap/buffered-io.c > @@ -744,8 +744,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, > poff, plen, srcmap); > if (status) > return status; > + iomap_set_range_uptodate(folio, poff, plen); > } > - iomap_set_range_uptodate(folio, poff, plen); > } while ((block_start += plen) < block_end); Um, what I meant was to just delete the iomap_set_range_uptodate() call in __iomap_write_begin() altogether. We'll call it soon enough in __iomap_write_end().
On 2024/8/17 12:48, Matthew Wilcox wrote: > On Mon, Aug 12, 2024 at 08:11:58PM +0800, Zhang Yi wrote: >> +++ b/fs/iomap/buffered-io.c >> @@ -744,8 +744,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, >> poff, plen, srcmap); >> if (status) >> return status; >> + iomap_set_range_uptodate(folio, poff, plen); >> } >> - iomap_set_range_uptodate(folio, poff, plen); >> } while ((block_start += plen) < block_end); > > Um, what I meant was to just delete the iomap_set_range_uptodate() > call in __iomap_write_begin() altogether. We'll call it soon enough in > __iomap_write_end(). > Yeah! Looks reasonable to me. Thanks, Yi.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index ac762de9a27f..96600405dbb5 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -744,8 +744,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos, poff, plen, srcmap); if (status) return status; + iomap_set_range_uptodate(folio, poff, plen); } - iomap_set_range_uptodate(folio, poff, plen); } while ((block_start += plen) < block_end); return 0;