| Message ID | 20240528164829.2105447-8-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Start moving write_begin/write_end out of aops | expand |
Hi Matthew,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.10-rc1 next-20240528]
[cannot apply to tytso-ext4/dev jack-fs/for_next hch-configfs/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Matthew-Wilcox-Oracle/fs-Introduce-buffered_write_operations/20240529-005213
base: linus/master
patch link: https://lore.kernel.org/r/20240528164829.2105447-8-willy%40infradead.org
patch subject: [PATCH 7/7] iomap: Return the folio from iomap_write_begin()
config: hexagon-allnoconfig (https://download.01.org/0day-ci/archive/20240529/202405290732.YLYLE1Zi-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project bafda89a0944d947fc4b3b5663185e07a397ac30)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240529/202405290732.YLYLE1Zi-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405290732.YLYLE1Zi-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from fs/iomap/buffered-io.c:9:
In file included from include/linux/iomap.h:7:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:10:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from fs/iomap/buffered-io.c:9:
In file included from include/linux/iomap.h:7:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from fs/iomap/buffered-io.c:9:
In file included from include/linux/iomap.h:7:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from fs/iomap/buffered-io.c:9:
In file included from include/linux/iomap.h:7:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> fs/iomap/buffered-io.c:802:7: warning: variable 'status' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
802 | if (!iomap_valid) {
| ^~~~~~~~~~~~
fs/iomap/buffered-io.c:826:17: note: uninitialized use occurs here
826 | return ERR_PTR(status);
| ^~~~~~
fs/iomap/buffered-io.c:802:3: note: remove the 'if' if its condition is always false
802 | if (!iomap_valid) {
| ^~~~~~~~~~~~~~~~~~~
803 | iter->iomap.flags |= IOMAP_F_STALE;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
804 | goto out_unlock;
| ~~~~~~~~~~~~~~~~
805 | }
| ~
fs/iomap/buffered-io.c:773:12: note: initialize the variable 'status' to silence this warning
773 | int status;
| ^
| = 0
8 warnings generated.
vim +802 fs/iomap/buffered-io.c
69f4a26c1e0c7c Gao Xiang 2021-08-03 766
07d07cfe38427e Matthew Wilcox (Oracle 2024-05-28 767) static struct folio *iomap_write_begin(struct iomap_iter *iter, loff_t pos,
07d07cfe38427e Matthew Wilcox (Oracle 2024-05-28 768) size_t len)
afc51aaa22f26c Darrick J. Wong 2019-07-15 769 {
471859f57d4253 Andreas Gruenbacher 2023-01-15 770 const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
fad0a1ab34f777 Christoph Hellwig 2021-08-10 771 const struct iomap *srcmap = iomap_iter_srcmap(iter);
d1bd0b4ebfe052 Matthew Wilcox (Oracle 2021-11-03 772) struct folio *folio;
07d07cfe38427e Matthew Wilcox (Oracle 2024-05-28 773) int status;
afc51aaa22f26c Darrick J. Wong 2019-07-15 774
1b5c1e36dc0e0f Christoph Hellwig 2021-08-10 775 BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
1b5c1e36dc0e0f Christoph Hellwig 2021-08-10 776 if (srcmap != &iter->iomap)
c039b997927263 Goldwyn Rodrigues 2019-10-18 777 BUG_ON(pos + len > srcmap->offset + srcmap->length);
afc51aaa22f26c Darrick J. Wong 2019-07-15 778
afc51aaa22f26c Darrick J. Wong 2019-07-15 779 if (fatal_signal_pending(current))
07d07cfe38427e Matthew Wilcox (Oracle 2024-05-28 780) return ERR_PTR(-EINTR);
afc51aaa22f26c Darrick J. Wong 2019-07-15 781
d454ab82bc7f4a Matthew Wilcox (Oracle 2021-12-09 782) if (!mapping_large_folio_support(iter->inode->i_mapping))
d454ab82bc7f4a Matthew Wilcox (Oracle 2021-12-09 783) len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
d454ab82bc7f4a Matthew Wilcox (Oracle 2021-12-09 784)
07c22b56685dd7 Andreas Gruenbacher 2023-01-15 785 folio = __iomap_get_folio(iter, pos, len);
9060bc4d3aca61 Andreas Gruenbacher 2023-01-15 786 if (IS_ERR(folio))
07d07cfe38427e Matthew Wilcox (Oracle 2024-05-28 787) return folio;
d7b64041164ca1 Dave Chinner 2022-11-29 788
d7b64041164ca1 Dave Chinner 2022-11-29 789 /*
d7b64041164ca1 Dave Chinner 2022-11-29 790 * Now we have a locked folio, before we do anything with it we need to
d7b64041164ca1 Dave Chinner 2022-11-29 791 * check that the iomap we have cached is not stale. The inode extent
d7b64041164ca1 Dave Chinner 2022-11-29 792 * mapping can change due to concurrent IO in flight (e.g.
d7b64041164ca1 Dave Chinner 2022-11-29 793 * IOMAP_UNWRITTEN state can change and memory reclaim could have
d7b64041164ca1 Dave Chinner 2022-11-29 794 * reclaimed a previously partially written page at this index after IO
d7b64041164ca1 Dave Chinner 2022-11-29 795 * completion before this write reaches this file offset) and hence we
d7b64041164ca1 Dave Chinner 2022-11-29 796 * could do the wrong thing here (zero a page range incorrectly or fail
d7b64041164ca1 Dave Chinner 2022-11-29 797 * to zero) and corrupt data.
d7b64041164ca1 Dave Chinner 2022-11-29 798 */
471859f57d4253 Andreas Gruenbacher 2023-01-15 799 if (folio_ops && folio_ops->iomap_valid) {
471859f57d4253 Andreas Gruenbacher 2023-01-15 800 bool iomap_valid = folio_ops->iomap_valid(iter->inode,
d7b64041164ca1 Dave Chinner 2022-11-29 801 &iter->iomap);
d7b64041164ca1 Dave Chinner 2022-11-29 @802 if (!iomap_valid) {
d7b64041164ca1 Dave Chinner 2022-11-29 803 iter->iomap.flags |= IOMAP_F_STALE;
d7b64041164ca1 Dave Chinner 2022-11-29 804 goto out_unlock;
d7b64041164ca1 Dave Chinner 2022-11-29 805 }
d7b64041164ca1 Dave Chinner 2022-11-29 806 }
d7b64041164ca1 Dave Chinner 2022-11-29 807
d454ab82bc7f4a Matthew Wilcox (Oracle 2021-12-09 808) if (pos + len > folio_pos(folio) + folio_size(folio))
d454ab82bc7f4a Matthew Wilcox (Oracle 2021-12-09 809) len = folio_pos(folio) + folio_size(folio) - pos;
afc51aaa22f26c Darrick J. Wong 2019-07-15 810
c039b997927263 Goldwyn Rodrigues 2019-10-18 811 if (srcmap->type == IOMAP_INLINE)
bc6123a84a71b5 Matthew Wilcox (Oracle 2021-05-02 812) status = iomap_write_begin_inline(iter, folio);
1b5c1e36dc0e0f Christoph Hellwig 2021-08-10 813 else if (srcmap->flags & IOMAP_F_BUFFER_HEAD)
d1bd0b4ebfe052 Matthew Wilcox (Oracle 2021-11-03 814) status = __block_write_begin_int(folio, pos, len, NULL, srcmap);
afc51aaa22f26c Darrick J. Wong 2019-07-15 815 else
bc6123a84a71b5 Matthew Wilcox (Oracle 2021-05-02 816) status = __iomap_write_begin(iter, pos, len, folio);
afc51aaa22f26c Darrick J. Wong 2019-07-15 817
afc51aaa22f26c Darrick J. Wong 2019-07-15 818 if (unlikely(status))
afc51aaa22f26c Darrick J. Wong 2019-07-15 819 goto out_unlock;
afc51aaa22f26c Darrick J. Wong 2019-07-15 820
07d07cfe38427e Matthew Wilcox (Oracle 2024-05-28 821) return folio;
afc51aaa22f26c Darrick J. Wong 2019-07-15 822
afc51aaa22f26c Darrick J. Wong 2019-07-15 823 out_unlock:
7a70a5085ed028 Andreas Gruenbacher 2023-01-15 824 __iomap_put_folio(iter, pos, 0, folio);
afc51aaa22f26c Darrick J. Wong 2019-07-15 825
07d07cfe38427e Matthew Wilcox (Oracle 2024-05-28 826) return ERR_PTR(status);
afc51aaa22f26c Darrick J. Wong 2019-07-15 827 }
afc51aaa22f26c Darrick J. Wong 2019-07-15 828
On Tue, May 28, 2024 at 05:48:28PM +0100, Matthew Wilcox (Oracle) wrote: > Use an ERR_PTR to return any error that may have occurred, otherwise > return the folio directly instead of returning it by reference. This > mirrors changes which are going into the filemap ->write_begin callbacks. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/iomap/buffered-io.c | 35 ++++++++++++++++------------------- > 1 file changed, 16 insertions(+), 19 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index c5802a459334..f0c40ac425ce 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -764,27 +764,27 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter, > return iomap_read_inline_data(iter, folio); > } > > -static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, > - size_t len, struct folio **foliop) > +static struct folio *iomap_write_begin(struct iomap_iter *iter, loff_t pos, > + size_t len) > { > const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops; > const struct iomap *srcmap = iomap_iter_srcmap(iter); > struct folio *folio; > - int status = 0; > + int status; Uninitialised return value. > > BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length); > if (srcmap != &iter->iomap) > BUG_ON(pos + len > srcmap->offset + srcmap->length); > > if (fatal_signal_pending(current)) > - return -EINTR; > + return ERR_PTR(-EINTR); > > if (!mapping_large_folio_support(iter->inode->i_mapping)) > len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos)); > > folio = __iomap_get_folio(iter, pos, len); > if (IS_ERR(folio)) > - return PTR_ERR(folio); > + return folio; > > /* > * Now we have a locked folio, before we do anything with it we need to > @@ -801,7 +801,6 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, > &iter->iomap); > if (!iomap_valid) { > iter->iomap.flags |= IOMAP_F_STALE; > - status = 0; > goto out_unlock; > } > } That looks wrong - status is now uninitialised when we jump to the error handling. This case needs to return "no error, no folio" so that the caller can detect the IOMAP_F_STALE flag and do the right thing.... > @@ -819,13 +818,12 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, > if (unlikely(status)) > goto out_unlock; > > - *foliop = folio; > - return 0; > + return folio; > > out_unlock: > __iomap_put_folio(iter, pos, 0, folio); > > - return status; > + return ERR_PTR(status); This returns the uninitialised status value.... > } > > static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len, > @@ -940,9 +938,10 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) > break; > } > > - status = iomap_write_begin(iter, pos, bytes, &folio); > - if (unlikely(status)) { > + folio = iomap_write_begin(iter, pos, bytes); > + if (IS_ERR(folio)) { > iomap_write_failed(iter->inode, pos, bytes); > + status = PTR_ERR(folio); > break; > } > if (iter->iomap.flags & IOMAP_F_STALE) So this will now fail the write rather than iterating again at the same offset with a new iomap. -Dave.
On Tue, May 28, 2024 at 05:48:28PM +0100, Matthew Wilcox (Oracle) wrote: > Use an ERR_PTR to return any error that may have occurred, otherwise > return the folio directly instead of returning it by reference. This > mirrors changes which are going into the filemap ->write_begin callbacks. Besides the mechanical errors pointed out by Dave I really like the new calling convention.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index c5802a459334..f0c40ac425ce 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -764,27 +764,27 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter, return iomap_read_inline_data(iter, folio); } -static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, - size_t len, struct folio **foliop) +static struct folio *iomap_write_begin(struct iomap_iter *iter, loff_t pos, + size_t len) { const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops; const struct iomap *srcmap = iomap_iter_srcmap(iter); struct folio *folio; - int status = 0; + int status; BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length); if (srcmap != &iter->iomap) BUG_ON(pos + len > srcmap->offset + srcmap->length); if (fatal_signal_pending(current)) - return -EINTR; + return ERR_PTR(-EINTR); if (!mapping_large_folio_support(iter->inode->i_mapping)) len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos)); folio = __iomap_get_folio(iter, pos, len); if (IS_ERR(folio)) - return PTR_ERR(folio); + return folio; /* * Now we have a locked folio, before we do anything with it we need to @@ -801,7 +801,6 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, &iter->iomap); if (!iomap_valid) { iter->iomap.flags |= IOMAP_F_STALE; - status = 0; goto out_unlock; } } @@ -819,13 +818,12 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, if (unlikely(status)) goto out_unlock; - *foliop = folio; - return 0; + return folio; out_unlock: __iomap_put_folio(iter, pos, 0, folio); - return status; + return ERR_PTR(status); } static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len, @@ -940,9 +938,10 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) break; } - status = iomap_write_begin(iter, pos, bytes, &folio); - if (unlikely(status)) { + folio = iomap_write_begin(iter, pos, bytes); + if (IS_ERR(folio)) { iomap_write_failed(iter->inode, pos, bytes); + status = PTR_ERR(folio); break; } if (iter->iomap.flags & IOMAP_F_STALE) @@ -1330,14 +1329,13 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter) do { struct folio *folio; - int status; size_t offset; size_t bytes = min_t(u64, SIZE_MAX, length); bool ret; - status = iomap_write_begin(iter, pos, bytes, &folio); - if (unlikely(status)) - return status; + folio = iomap_write_begin(iter, pos, bytes); + if (IS_ERR(folio)) + return PTR_ERR(folio); if (iomap->flags & IOMAP_F_STALE) break; @@ -1393,14 +1391,13 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) do { struct folio *folio; - int status; size_t offset; size_t bytes = min_t(u64, SIZE_MAX, length); bool ret; - status = iomap_write_begin(iter, pos, bytes, &folio); - if (status) - return status; + folio = iomap_write_begin(iter, pos, bytes); + if (IS_ERR(folio)) + return PTR_ERR(folio); if (iter->iomap.flags & IOMAP_F_STALE) break;
Use an ERR_PTR to return any error that may have occurred, otherwise return the folio directly instead of returning it by reference. This mirrors changes which are going into the filemap ->write_begin callbacks. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/iomap/buffered-io.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)