Re: [PATCH 09/14] iomap: Change iomap_write_begin calling convention

From: Matthew Wilcox <willy@infradead.org>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-mm@kvack.org
Subject: Re: [PATCH 09/14] iomap: Change iomap_write_begin calling convention
Date: Wed, 14 Oct 2020 19:08:28 +0100	[thread overview]
Message-ID: <20201014180828.GT20115@casper.infradead.org> (raw)
In-Reply-To: <20201014174158.GS20115@casper.infradead.org>

On Wed, Oct 14, 2020 at 06:41:58PM +0100, Matthew Wilcox wrote:
> On Wed, Oct 14, 2020 at 09:47:44AM -0700, Darrick J. Wong wrote:
> > > -static int
> > > -iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
> > > -		struct page **pagep, struct iomap *iomap, struct iomap *srcmap)
> > > +static ssize_t iomap_write_begin(struct inode *inode, loff_t pos, loff_t len,
> > > +		unsigned flags, struct page **pagep, struct iomap *iomap,
> > 
> > loff_t len?  You've been using size_t (ssize_t?) for length elsewhere,
> > can't return more than ssize_t, and afaik MAX_RW_COUNT will never go
> > larger than 2GB so I'm confused about types here...?
> 
> Yes, you're right.  This one should be size_t.
> 
> > >  	if (page_ops && page_ops->page_prepare) {
> > > +		if (len > UINT_MAX)
> > > +			len = UINT_MAX;
> > 
> > I'm not especially familiar with page_prepare (since it's a gfs2 thing);
> > why do you clamp len to UINT_MAX here?
> 
> The len parameter of ->page_prepare is an unsigned int.  I don't want
> a 1<<32+1 byte I/O to be seen as a 1 byte I/O.  We could upgrade the
> parameter to size_t from unsigned int?

OK, here's what I have -- two patches (copy-and-wasted):

    iomap: Prepare page ops for larger I/Os
    
    Just adjust the types for the page_prepare() and page_done() callbacks
    to size_t from unsigned int.  While I don't see individual pages that
    large in our near future, it's convenient to be able to pass in larger
    lengths and has no cost.
    
    Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 0f69fbd4af66..9a94d7e58199 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1028,7 +1028,7 @@ static void gfs2_write_unlock(struct inode *inode)
 }
 
 static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,
-                                  unsigned len, struct iomap *iomap)
+                                  size_t len, struct iomap *iomap)
 {
        unsigned int blockmask = i_blocksize(inode) - 1;
        struct gfs2_sbd *sdp = GFS2_SB(inode);
@@ -1039,7 +1039,7 @@ static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,
 }
 
 static void gfs2_iomap_page_done(struct inode *inode, loff_t pos,
-                                unsigned copied, struct page *page,
+                                size_t copied, struct page *page,
                                 struct iomap *iomap)
 {
        struct gfs2_trans *tr = current->journal_info;
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 4d1d3c3469e9..d3b06bffd996 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -106,9 +106,9 @@ iomap_sector(struct iomap *iomap, loff_t pos)
  * associated page could not be obtained.
  */
 struct iomap_page_ops {
-       int (*page_prepare)(struct inode *inode, loff_t pos, unsigned len,
+       int (*page_prepare)(struct inode *inode, loff_t pos, size_t len,
                        struct iomap *iomap);
-       void (*page_done)(struct inode *inode, loff_t pos, unsigned copied,
+       void (*page_done)(struct inode *inode, loff_t pos, size_t copied,
                        struct page *page, struct iomap *iomap);
 };
 


The other is an amendment of this:

-static int
-iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags
,
-               struct page **pagep, struct iomap *iomap, struct iomap *srcmap)
+static ssize_t iomap_write_begin(struct inode *inode, loff_t pos, size_t len,
+               unsigned flags, struct page **pagep, struct iomap *iomap,
+               struct iomap *srcmap)

and I drop the clamp to UINT_MAX.  I also added:

-               unsigned long offset;   /* Offset into pagecache page */
-               unsigned long bytes;    /* Bytes to write to page */
+               size_t offset;          /* Offset into pagecache page */
+               size_t bytes;           /* Bytes to write to page */

which won't change anything, but does fit the general pattern of
using size_t for a byte count of in-memory things.  It matches
iomap_unshare_actor(), for example.
