| Message ID | 20200824145511.10500-9-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | THP iomap patches for 5.10 | expand |
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.
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.
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.
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 --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();
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(-)