diff mbox series

[8/9] iomap: Convert iomap_write_end types

Message ID 20200824145511.10500-9-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series THP iomap patches for 5.10 | expand

Commit Message

Matthew Wilcox Aug. 24, 2020, 2:55 p.m. UTC
iomap_write_end cannot return an error, so switch it to return
size_t instead of int and remove the error checking from the callers.
Also convert the arguments to size_t from unsigned int, in case anyone
ever wants to support a page size larger than 2GB.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/iomap/buffered-io.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

Comments

Dave Chinner Aug. 25, 2020, 12:12 a.m. UTC | #1
On Mon, Aug 24, 2020 at 03:55:09PM +0100, Matthew Wilcox (Oracle) wrote:
> iomap_write_end cannot return an error, so switch it to return
> size_t instead of int and remove the error checking from the callers.
> Also convert the arguments to size_t from unsigned int, in case anyone
> ever wants to support a page size larger than 2GB.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/iomap/buffered-io.c | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 8c6187a6f34e..7f618ab4b11e 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -666,9 +666,8 @@ iomap_set_page_dirty(struct page *page)
>  }
>  EXPORT_SYMBOL_GPL(iomap_set_page_dirty);
>  
> -static int
> -__iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
> -		unsigned copied, struct page *page)
> +static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
> +		size_t copied, struct page *page)
>  {

Please leave the function declarations formatted the same way as
they currently are. They are done that way intentionally so it is
easy to grep for function definitions. Not to mention is't much
easier to read than when the function name is commingled into the
multiline paramener list like...

> -static int
> -iomap_write_end(struct inode *inode, loff_t pos, unsigned len, unsigned copied,
> -		struct page *page, struct iomap *iomap, struct iomap *srcmap)
> +/* Returns the number of bytes copied.  May be 0.  Cannot be an errno. */
> +static size_t iomap_write_end(struct inode *inode, loff_t pos, size_t len,
> +		size_t copied, struct page *page, struct iomap *iomap,
> +		struct iomap *srcmap)

... this.

Otherwise the code looks fine.

Cheers,

Dave.
Matthew Wilcox Aug. 25, 2020, 1:06 a.m. UTC | #2
On Tue, Aug 25, 2020 at 10:12:23AM +1000, Dave Chinner wrote:
> > -static int
> > -__iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
> > -		unsigned copied, struct page *page)
> > +static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
> > +		size_t copied, struct page *page)
> >  {
> 
> Please leave the function declarations formatted the same way as
> they currently are. They are done that way intentionally so it is
> easy to grep for function definitions. Not to mention is't much
> easier to read than when the function name is commingled into the
> multiline paramener list like...

I understand that's true for XFS, but it's not true throughout the
rest of the kernel.  This file isn't even consistent:

buffered-io.c:static inline struct iomap_page *to_iomap_page(struct page *page)
buffered-io.c:static inline bool iomap_block_needs_zeroing(struct inode
buffered-io.c:static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset,
buffered-io.c:static void iomap_writepage_end_bio(struct bio *bio)
buffered-io.c:static int __init iomap_init(void)

(i just grepped for ^static so there're other functions not covered by this)

The other fs/iomap/ files are equally inconsistent.
Dave Chinner Aug. 25, 2020, 1:33 a.m. UTC | #3
On Tue, Aug 25, 2020 at 02:06:05AM +0100, Matthew Wilcox wrote:
> On Tue, Aug 25, 2020 at 10:12:23AM +1000, Dave Chinner wrote:
> > > -static int
> > > -__iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
> > > -		unsigned copied, struct page *page)
> > > +static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
> > > +		size_t copied, struct page *page)
> > >  {
> > 
> > Please leave the function declarations formatted the same way as
> > they currently are. They are done that way intentionally so it is
> > easy to grep for function definitions. Not to mention is't much
> > easier to read than when the function name is commingled into the
> > multiline paramener list like...
> 
> I understand that's true for XFS, but it's not true throughout the
> rest of the kernel. 

What other code does is irrelevant. I'm trying to maintain and
improve the consistency of the format used for the fs/iomap code.

> This file isn't even consistent:
> 
> buffered-io.c:static inline struct iomap_page *to_iomap_page(struct page *page)
> buffered-io.c:static inline bool iomap_block_needs_zeroing(struct inode
> buffered-io.c:static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset,
> buffered-io.c:static void iomap_writepage_end_bio(struct bio *bio)
> buffered-io.c:static int __init iomap_init(void)
> 
> (i just grepped for ^static so there're other functions not covered by this)

5 functions that have that format, compared to 45 that do have the
formatting I asked you to retain. It think it's pretty clear which
way consistency lies here...

> The other fs/iomap/ files are equally inconsistent.

Inconsistency always occurs when multiple people modify the same
code. Often that's simply because reviewers haven't noticed the
inconsistency - it's certainly not intentional.

Saying "No, I'm going to make the code less consistent because it's
already slightly inconsistent" is, IMO, not a valid response to a
review request to conform to the existing code layout in that file,
especially if it improves the consistency of the code being
modified. That's really not negotiable....

Cheers,

Dave.
Christoph Hellwig Aug. 27, 2020, 8:41 a.m. UTC | #4
On Mon, Aug 24, 2020 at 03:55:09PM +0100, Matthew Wilcox (Oracle) wrote:
> iomap_write_end cannot return an error, so switch it to return
> size_t instead of int and remove the error checking from the callers.
> Also convert the arguments to size_t from unsigned int, in case anyone
> ever wants to support a page size larger than 2GB.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 8c6187a6f34e..7f618ab4b11e 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -666,9 +666,8 @@  iomap_set_page_dirty(struct page *page)
 }
 EXPORT_SYMBOL_GPL(iomap_set_page_dirty);
 
-static int
-__iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
-		unsigned copied, struct page *page)
+static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
+		size_t copied, struct page *page)
 {
 	flush_dcache_page(page);
 
@@ -690,9 +689,8 @@  __iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
 	return copied;
 }
 
-static int
-iomap_write_end_inline(struct inode *inode, struct page *page,
-		struct iomap *iomap, loff_t pos, unsigned copied)
+static size_t iomap_write_end_inline(struct inode *inode, struct page *page,
+		struct iomap *iomap, loff_t pos, size_t copied)
 {
 	void *addr;
 
@@ -708,13 +706,14 @@  iomap_write_end_inline(struct inode *inode, struct page *page,
 	return copied;
 }
 
-static int
-iomap_write_end(struct inode *inode, loff_t pos, unsigned len, unsigned copied,
-		struct page *page, struct iomap *iomap, struct iomap *srcmap)
+/* Returns the number of bytes copied.  May be 0.  Cannot be an errno. */
+static size_t iomap_write_end(struct inode *inode, loff_t pos, size_t len,
+		size_t copied, struct page *page, struct iomap *iomap,
+		struct iomap *srcmap)
 {
 	const struct iomap_page_ops *page_ops = iomap->page_ops;
 	loff_t old_size = inode->i_size;
-	int ret;
+	size_t ret;
 
 	if (srcmap->type == IOMAP_INLINE) {
 		ret = iomap_write_end_inline(inode, page, iomap, pos, copied);
@@ -793,11 +792,8 @@  iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 
 		copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
 
-		status = iomap_write_end(inode, pos, bytes, copied, page, iomap,
+		copied = iomap_write_end(inode, pos, bytes, copied, page, iomap,
 				srcmap);
-		if (unlikely(status < 0))
-			break;
-		copied = status;
 
 		cond_resched();
 
@@ -871,11 +867,8 @@  iomap_unshare_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 
 		status = iomap_write_end(inode, pos, bytes, bytes, page, iomap,
 				srcmap);
-		if (unlikely(status <= 0)) {
-			if (WARN_ON_ONCE(status == 0))
-				return -EIO;
-			return status;
-		}
+		if (WARN_ON_ONCE(status == 0))
+			return -EIO;
 
 		cond_resched();