Re: [PATCH 7/7] iomap: Return the folio from iomap_write

All of lore.kernel.org
 help / color / mirror / Atom feed

* Re: [PATCH 7/7] iomap: Return the folio from iomap_write_begin()
@ 2024-05-29 16:39 kernel test robot
  0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2024-05-29 16:39 UTC (permalink / raw)
  To: oe-kbuild; +Cc: lkp, Dan Carpenter

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240528164829.2105447-8-willy@infradead.org>
References: <20240528164829.2105447-8-willy@infradead.org>
TO: "Matthew Wilcox (Oracle)" <willy@infradead.org>
TO: Christoph Hellwig <hch@lst.de>
CC: "Matthew Wilcox (Oracle)" <willy@infradead.org>
CC: linux-fsdevel@vger.kernel.org
CC: linux-ext4@vger.kernel.org

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-20240529]
[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()
:::::: branch date: 24 hours ago
:::::: commit date: 24 hours ago
config: openrisc-randconfig-r081-20240529 (https://download.01.org/0day-ci/archive/20240530/202405300028.ZTOIuctX-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0

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>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202405300028.ZTOIuctX-lkp@intel.com/

smatch warnings:
fs/iomap/buffered-io.c:826 iomap_write_begin() error: uninitialized symbol 'status'.

vim +/status +826 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  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 0/7] Start moving write_begin/write_end out of aops
@ 2024-05-28 16:48 Matthew Wilcox (Oracle)
  2024-05-28 16:48 ` [PATCH 7/7] iomap: Return the folio from iomap_write_begin() Matthew Wilcox (Oracle)
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox (Oracle) @ 2024-05-28 16:48 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Matthew Wilcox (Oracle), linux-fsdevel, linux-ext4

Christoph wants to remove write_begin/write_end from aops and pass them
to filemap as callback functions.  Here's one possible route to do this.
I combined it with the folio conversion (because why touch the same code
twice?) and tweaked some of the other things (support for ridiculously
large folios with size_t lengths, remove the need to initialise fsdata
by passing only a pointer to the fsdata pointer).  And then I converted
ext4, which is probably the worst filesystem to convert because it needs
three different bwops.  Most fs will only need one.

Not written yet: convert all the other fs, remove wrappers.

v2:
 - Redo how we pass fsdata around so it can persist across multiple
   invocations of filemap_perform_write()
 - Add ext2
 - Minor tweak to iomap

This is against 2bfcfd584ff5 (Linus current head) and will conflict with
other patches in flight.

Matthew Wilcox (Oracle) (7):
  fs: Introduce buffered_write_operations
  fs: Supply optional buffered_write_operations in buffer.c
  buffer: Add buffer_write_begin, buffer_write_end and
    __buffer_write_end
  fs: Add filemap_symlink()
  ext2: Convert to buffered_write_operations
  ext4: Convert to buffered_write_operations
  iomap: Return the folio from iomap_write_begin()

 Documentation/filesystems/locking.rst |  23 ++++
 fs/buffer.c                           | 158 ++++++++++++++++++--------
 fs/ext2/ext2.h                        |   1 +
 fs/ext2/file.c                        |   4 +-
 fs/ext2/inode.c                       |  55 ++++-----
 fs/ext2/namei.c                       |   2 +-
 fs/ext4/ext4.h                        |  24 ++--
 fs/ext4/file.c                        |  12 +-
 fs/ext4/inline.c                      |  66 +++++------
 fs/ext4/inode.c                       | 134 ++++++++++------------
 fs/iomap/buffered-io.c                |  35 +++---
 fs/jfs/file.c                         |   3 +-
 fs/namei.c                            |  25 ++++
 fs/ramfs/file-mmu.c                   |   3 +-
 fs/ufs/file.c                         |   2 +-
 include/linux/buffer_head.h           |  28 ++++-
 include/linux/fs.h                    |   3 -
 include/linux/pagemap.h               |  23 ++++
 mm/filemap.c                          |  77 ++++++++-----
 19 files changed, 417 insertions(+), 261 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 7/7] iomap: Return the folio from iomap_write_begin()
  2024-05-28 16:48 [PATCH 0/7] Start moving write_begin/write_end out of aops Matthew Wilcox (Oracle)
@ 2024-05-28 16:48 ` Matthew Wilcox (Oracle)
  2024-05-28 23:31   ` kernel test robot
                     ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Matthew Wilcox (Oracle) @ 2024-05-28 16:48 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Matthew Wilcox (Oracle), linux-fsdevel, linux-ext4

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;
 
 	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;
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 7/7] iomap: Return the folio from iomap_write_begin()
  2024-05-28 16:48 ` [PATCH 7/7] iomap: Return the folio from iomap_write_begin() Matthew Wilcox (Oracle)
@ 2024-05-28 23:31   ` kernel test robot
  2024-05-28 23:44   ` Dave Chinner
  2024-05-29  5:25   ` Christoph Hellwig
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2024-05-28 23:31 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), Christoph Hellwig
  Cc: llvm, oe-kbuild-all, Matthew Wilcox (Oracle), linux-fsdevel,
	linux-ext4

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  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 7/7] iomap: Return the folio from iomap_write_begin()
  2024-05-28 16:48 ` [PATCH 7/7] iomap: Return the folio from iomap_write_begin() Matthew Wilcox (Oracle)
  2024-05-28 23:31   ` kernel test robot
@ 2024-05-28 23:44   ` Dave Chinner
  2024-05-29  5:25   ` Christoph Hellwig
  2 siblings, 0 replies; 5+ messages in thread
From: Dave Chinner @ 2024-05-28 23:44 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle); +Cc: Christoph Hellwig, linux-fsdevel, linux-ext4

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.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 7/7] iomap: Return the folio from iomap_write_begin()
  2024-05-28 16:48 ` [PATCH 7/7] iomap: Return the folio from iomap_write_begin() Matthew Wilcox (Oracle)
  2024-05-28 23:31   ` kernel test robot
  2024-05-28 23:44   ` Dave Chinner
@ 2024-05-29  5:25   ` Christoph Hellwig
  2 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2024-05-29  5:25 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle); +Cc: Christoph Hellwig, linux-fsdevel, linux-ext4

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-05-29 16:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-29 16:39 [PATCH 7/7] iomap: Return the folio from iomap_write_begin() kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2024-05-28 16:48 [PATCH 0/7] Start moving write_begin/write_end out of aops Matthew Wilcox (Oracle)
2024-05-28 16:48 ` [PATCH 7/7] iomap: Return the folio from iomap_write_begin() Matthew Wilcox (Oracle)
2024-05-28 23:31   ` kernel test robot
2024-05-28 23:44   ` Dave Chinner
2024-05-29  5:25   ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.