| Message ID | 20200824145511.10500-10-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:10PM +0100, Matthew Wilcox (Oracle) wrote: > Pass the full length to iomap_zero() and dax_iomap_zero(), and have > them return how many bytes they actually handled. This is preparatory > work for handling THP, although it looks like DAX could actually take > advantage of it if there's a larger contiguous area. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/dax.c | 13 ++++++------- > fs/iomap/buffered-io.c | 33 +++++++++++++++------------------ > include/linux/dax.h | 3 +-- > 3 files changed, 22 insertions(+), 27 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index 95341af1a966..f2b912cb034e 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1037,18 +1037,18 @@ static vm_fault_t dax_load_hole(struct xa_state *xas, > return ret; > } > > -int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, > - struct iomap *iomap) > +loff_t dax_iomap_zero(loff_t pos, u64 length, struct iomap *iomap) > { > sector_t sector = iomap_sector(iomap, pos & PAGE_MASK); > pgoff_t pgoff; > long rc, id; > void *kaddr; > bool page_aligned = false; > - > + unsigned offset = offset_in_page(pos); > + unsigned size = min_t(u64, PAGE_SIZE - offset, length); > > if (IS_ALIGNED(sector << SECTOR_SHIFT, PAGE_SIZE) && > - IS_ALIGNED(size, PAGE_SIZE)) > + (size == PAGE_SIZE)) > page_aligned = true; > > rc = bdev_dax_pgoff(iomap->bdev, sector, PAGE_SIZE, &pgoff); > @@ -1058,8 +1058,7 @@ int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, > id = dax_read_lock(); > > if (page_aligned) > - rc = dax_zero_page_range(iomap->dax_dev, pgoff, > - size >> PAGE_SHIFT); > + rc = dax_zero_page_range(iomap->dax_dev, pgoff, 1); > else > rc = dax_direct_access(iomap->dax_dev, pgoff, 1, &kaddr, NULL); > if (rc < 0) { > @@ -1072,7 +1071,7 @@ int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, > dax_flush(iomap->dax_dev, kaddr + offset, size); > } > dax_read_unlock(id); > - return 0; > + return size; > } > > static loff_t > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 7f618ab4b11e..2dba054095e8 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -901,11 +901,13 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > } > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > -static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, > - unsigned bytes, struct iomap *iomap, struct iomap *srcmap) > +static loff_t iomap_zero(struct inode *inode, loff_t pos, u64 length, > + struct iomap *iomap, struct iomap *srcmap) While you are touching this, please convert it back to: static loff_t iomap_zero(struct inode *inode, loff_t pos, u64 length, struct iomap *iomap, struct iomap *srcmap) > { > struct page *page; > int status; > + unsigned offset = offset_in_page(pos); > + unsigned bytes = min_t(u64, PAGE_SIZE - offset, length); > > status = iomap_write_begin(inode, pos, bytes, 0, &page, iomap, srcmap); > if (status) > @@ -917,38 +919,33 @@ static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, > return iomap_write_end(inode, pos, bytes, bytes, page, iomap, srcmap); > } > > -static loff_t > -iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, > - void *data, struct iomap *iomap, struct iomap *srcmap) > +static loff_t iomap_zero_range_actor(struct inode *inode, loff_t pos, > + loff_t length, void *data, struct iomap *iomap, > + struct iomap *srcmap) And leave this as it was formatted. /me missed this when iomap_readahead() was introduced, too. > { > bool *did_zero = data; > loff_t written = 0; > - int status; > > /* already zeroed? we're done. */ > if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > - return count; > + return length; > > do { > - unsigned offset, bytes; > - > - offset = offset_in_page(pos); > - bytes = min_t(loff_t, PAGE_SIZE - offset, count); > + loff_t bytes; > > if (IS_DAX(inode)) > - status = dax_iomap_zero(pos, offset, bytes, iomap); > + bytes = dax_iomap_zero(pos, length, iomap); Hmmm. everything is loff_t here, but the callers are defining length as u64, not loff_t. Is there a potential sign conversion problem here? (sure 64 bit is way beyond anything we'll pass here, but...) Cheers, Dave.
On Tue, Aug 25, 2020 at 10:27:35AM +1000, Dave Chinner wrote: > > do { > > - unsigned offset, bytes; > > - > > - offset = offset_in_page(pos); > > - bytes = min_t(loff_t, PAGE_SIZE - offset, count); > > + loff_t bytes; > > > > if (IS_DAX(inode)) > > - status = dax_iomap_zero(pos, offset, bytes, iomap); > > + bytes = dax_iomap_zero(pos, length, iomap); > > Hmmm. everything is loff_t here, but the callers are defining length > as u64, not loff_t. Is there a potential sign conversion problem > here? (sure 64 bit is way beyond anything we'll pass here, but...) I've gone back and forth on the correct type for 'length' a few times. size_t is too small (not for zeroing, but for seek()). An unsigned type seems right -- a length can't be negative, and we don't want to give the impression that it can. But the return value from these functions definitely needs to be signed so we can represent an error. So a u64 length with an loff_t return type feels like the best solution. And the upper layers have to promise not to pass in a length that's more than 2^63-1. I have a patch set which starts the conversion process for all the actors over to using u64 for the length. Obviously, that's not mixed in with the THP patchset.
On Mon, Aug 24, 2020 at 09:35:59PM -0600, Andreas Dilger wrote: > On Aug 24, 2020, at 9:26 PM, Matthew Wilcox <willy@infradead.org> wrote: > > > > On Tue, Aug 25, 2020 at 10:27:35AM +1000, Dave Chinner wrote: > >>> do { > >>> - unsigned offset, bytes; > >>> - > >>> - offset = offset_in_page(pos); > >>> - bytes = min_t(loff_t, PAGE_SIZE - offset, count); > >>> + loff_t bytes; > >>> > >>> if (IS_DAX(inode)) > >>> - status = dax_iomap_zero(pos, offset, bytes, iomap); > >>> + bytes = dax_iomap_zero(pos, length, iomap); > >> > >> Hmmm. everything is loff_t here, but the callers are defining length > >> as u64, not loff_t. Is there a potential sign conversion problem > >> here? (sure 64 bit is way beyond anything we'll pass here, but...) > > > > I've gone back and forth on the correct type for 'length' a few times. > > size_t is too small (not for zeroing, but for seek()). An unsigned type > > seems right -- a length can't be negative, and we don't want to give > > the impression that it can. But the return value from these functions > > definitely needs to be signed so we can represent an error. So a u64 > > length with an loff_t return type feels like the best solution. And > > the upper layers have to promise not to pass in a length that's more > > than 2^63-1. > > The problem with allowing a u64 as the length is that it leads to the > possibility of an argument value that cannot be returned. Checking > length < 0 is not worse than checking length > 0x7ffffffffffffff, > and has the benefit of consistency with the other argument types and > signs... I think the problem here is that we have no guaranteed 64 bit size type. when that was the case with off_t, we created loff_t to always represent a 64 bit offset value. However, we never created one for the count/size that is passed alongside loff_t in many places - it was said that "syscalls are limited to 32 bit sizes" and "size_t is 64 bit on 64 bit platforms" and so on and so we still don't have a clean way to pass 64 bit sizes through the IO path. We've been living with this shitty situation for a long time now, so perhaps it's time for us to define lsize_t for 64 bit lengths and start using that everywhere that needs a 64 bit clean path through the code, regardless of whether the arch is 32 or 64 bit... Thoughts? -Dave.
On Tue, Aug 25, 2020 at 02:27:11PM +1000, Dave Chinner wrote: > On Mon, Aug 24, 2020 at 09:35:59PM -0600, Andreas Dilger wrote: > > On Aug 24, 2020, at 9:26 PM, Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Tue, Aug 25, 2020 at 10:27:35AM +1000, Dave Chinner wrote: > > >>> do { > > >>> - unsigned offset, bytes; > > >>> - > > >>> - offset = offset_in_page(pos); > > >>> - bytes = min_t(loff_t, PAGE_SIZE - offset, count); > > >>> + loff_t bytes; > > >>> > > >>> if (IS_DAX(inode)) > > >>> - status = dax_iomap_zero(pos, offset, bytes, iomap); > > >>> + bytes = dax_iomap_zero(pos, length, iomap); > > >> > > >> Hmmm. everything is loff_t here, but the callers are defining length > > >> as u64, not loff_t. Is there a potential sign conversion problem > > >> here? (sure 64 bit is way beyond anything we'll pass here, but...) > > > > > > I've gone back and forth on the correct type for 'length' a few times. > > > size_t is too small (not for zeroing, but for seek()). An unsigned type > > > seems right -- a length can't be negative, and we don't want to give > > > the impression that it can. But the return value from these functions > > > definitely needs to be signed so we can represent an error. So a u64 > > > length with an loff_t return type feels like the best solution. And > > > the upper layers have to promise not to pass in a length that's more > > > than 2^63-1. > > > > The problem with allowing a u64 as the length is that it leads to the > > possibility of an argument value that cannot be returned. Checking > > length < 0 is not worse than checking length > 0x7ffffffffffffff, > > and has the benefit of consistency with the other argument types and > > signs... The callee should just trust that the caller isn't going to do that. File sizes can't be more than 2^63-1 bytes, so an extent of a file also can't be more than 2^63-1 bytes. > I think the problem here is that we have no guaranteed 64 bit size > type. when that was the case with off_t, we created loff_t to always > represent a 64 bit offset value. However, we never created one for > the count/size that is passed alongside loff_t in many places - it > was said that "syscalls are limited to 32 bit sizes" and > "size_t is 64 bit on 64 bit platforms" and so on and so we still > don't have a clean way to pass 64 bit sizes through the IO path. > > We've been living with this shitty situation for a long time now, so > perhaps it's time for us to define lsize_t for 64 bit lengths and > start using that everywhere that needs a 64 bit clean path > through the code, regardless of whether the arch is 32 or 64 bit... > > Thoughts? I don't think the THP patches should be blocked on this expedition. We have a guaranteed 64 bit type -- it's u64. I don't think defining lsize_t is going to fix anything. The next big problem to fix will be supporting storage >16EiB, but I think that's a project that can start in ten years and still be here before anyone but the TLAs have that much storage in a single device. Any objection to leaving this patch as-is with a u64 length?
On Tue, Aug 25, 2020 at 01:40:24PM +0100, Matthew Wilcox wrote:
> Any objection to leaving this patch as-is with a u64 length?
No objection here - I just wanted to make sure that signed/unsigned
overflow was not going to be an issue...
Cheers,
Dave.
On Mon, Aug 24, 2020 at 03:55:10PM +0100, Matthew Wilcox (Oracle) wrote: > Pass the full length to iomap_zero() and dax_iomap_zero(), and have > them return how many bytes they actually handled. This is preparatory > work for handling THP, although it looks like DAX could actually take > advantage of it if there's a larger contiguous area. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/dax.c | 13 ++++++------- > fs/iomap/buffered-io.c | 33 +++++++++++++++------------------ > include/linux/dax.h | 3 +-- > 3 files changed, 22 insertions(+), 27 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index 95341af1a966..f2b912cb034e 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1037,18 +1037,18 @@ static vm_fault_t dax_load_hole(struct xa_state *xas, > return ret; > } > > -int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, > - struct iomap *iomap) > +loff_t dax_iomap_zero(loff_t pos, u64 length, struct iomap *iomap) Sorry for my ultra-slow response to this. The u64 length seems ok to me (or uint64_t, I don't care all /that/ much), but using loff_t as a return type bothers me because I see that and think that this function is returning a new file offset, e.g. (pos + number of bytes zeroed). So please, let's use s64 or something that isn't so misleading. FWIW, Linus also[0] doesn't[1] like using loff_t for the number of bytes copied. --D [0] https://lore.kernel.org/linux-fsdevel/CAHk-=wgcPAfOSigMf0xwaGfVjw413XN3UPATwYWHrss+QuivhQ@mail.gmail.com/ [1] https://lore.kernel.org/linux-fsdevel/CAHk-=wgvROUnrEVADVR_zTHY8NmYo-_jVjV37O1MdDm2de+Lmw@mail.gmail.com/ > { > sector_t sector = iomap_sector(iomap, pos & PAGE_MASK); > pgoff_t pgoff; > long rc, id; > void *kaddr; > bool page_aligned = false; > - > + unsigned offset = offset_in_page(pos); > + unsigned size = min_t(u64, PAGE_SIZE - offset, length); > > if (IS_ALIGNED(sector << SECTOR_SHIFT, PAGE_SIZE) && > - IS_ALIGNED(size, PAGE_SIZE)) > + (size == PAGE_SIZE)) > page_aligned = true; > > rc = bdev_dax_pgoff(iomap->bdev, sector, PAGE_SIZE, &pgoff); > @@ -1058,8 +1058,7 @@ int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, > id = dax_read_lock(); > > if (page_aligned) > - rc = dax_zero_page_range(iomap->dax_dev, pgoff, > - size >> PAGE_SHIFT); > + rc = dax_zero_page_range(iomap->dax_dev, pgoff, 1); > else > rc = dax_direct_access(iomap->dax_dev, pgoff, 1, &kaddr, NULL); > if (rc < 0) { > @@ -1072,7 +1071,7 @@ int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, > dax_flush(iomap->dax_dev, kaddr + offset, size); > } > dax_read_unlock(id); > - return 0; > + return size; > } > > static loff_t > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 7f618ab4b11e..2dba054095e8 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -901,11 +901,13 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > } > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > -static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, > - unsigned bytes, struct iomap *iomap, struct iomap *srcmap) > +static loff_t iomap_zero(struct inode *inode, loff_t pos, u64 length, > + struct iomap *iomap, struct iomap *srcmap) > { > struct page *page; > int status; > + unsigned offset = offset_in_page(pos); > + unsigned bytes = min_t(u64, PAGE_SIZE - offset, length); > > status = iomap_write_begin(inode, pos, bytes, 0, &page, iomap, srcmap); > if (status) > @@ -917,38 +919,33 @@ static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, > return iomap_write_end(inode, pos, bytes, bytes, page, iomap, srcmap); > } > > -static loff_t > -iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, > - void *data, struct iomap *iomap, struct iomap *srcmap) > +static loff_t iomap_zero_range_actor(struct inode *inode, loff_t pos, > + loff_t length, void *data, struct iomap *iomap, > + struct iomap *srcmap) > { > bool *did_zero = data; > loff_t written = 0; > - int status; > > /* already zeroed? we're done. */ > if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > - return count; > + return length; > > do { > - unsigned offset, bytes; > - > - offset = offset_in_page(pos); > - bytes = min_t(loff_t, PAGE_SIZE - offset, count); > + loff_t bytes; > > if (IS_DAX(inode)) > - status = dax_iomap_zero(pos, offset, bytes, iomap); > + bytes = dax_iomap_zero(pos, length, iomap); > else > - status = iomap_zero(inode, pos, offset, bytes, iomap, > - srcmap); > - if (status < 0) > - return status; > + bytes = iomap_zero(inode, pos, length, iomap, srcmap); > + if (bytes < 0) > + return bytes; > > pos += bytes; > - count -= bytes; > + length -= bytes; > written += bytes; > if (did_zero) > *did_zero = true; > - } while (count > 0); > + } while (length > 0); > > return written; > } > diff --git a/include/linux/dax.h b/include/linux/dax.h > index 6904d4e0b2e0..80f17946f940 100644 > --- a/include/linux/dax.h > +++ b/include/linux/dax.h > @@ -214,8 +214,7 @@ vm_fault_t dax_finish_sync_fault(struct vm_fault *vmf, > int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index); > int dax_invalidate_mapping_entry_sync(struct address_space *mapping, > pgoff_t index); > -int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, > - struct iomap *iomap); > +loff_t dax_iomap_zero(loff_t pos, u64 length, struct iomap *iomap); > static inline bool dax_mapping(struct address_space *mapping) > { > return mapping->host && IS_DAX(mapping->host); > -- > 2.28.0 >
On Tue, Aug 25, 2020 at 03:23:55PM -0700, Darrick J. Wong wrote: > Sorry for my ultra-slow response to this. The u64 length seems ok to me > (or uint64_t, I don't care all /that/ much), but using loff_t as a > return type bothers me because I see that and think that this function > is returning a new file offset, e.g. (pos + number of bytes zeroed). > > So please, let's use s64 or something that isn't so misleading. > > FWIW, Linus also[0] doesn't[1] like using loff_t for the number of bytes > copied. Let's just switch to u64 and s64 then. Unless we want to come up with our own typedefs.
diff --git a/fs/dax.c b/fs/dax.c index 95341af1a966..f2b912cb034e 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -1037,18 +1037,18 @@ static vm_fault_t dax_load_hole(struct xa_state *xas, return ret; } -int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, - struct iomap *iomap) +loff_t dax_iomap_zero(loff_t pos, u64 length, struct iomap *iomap) { sector_t sector = iomap_sector(iomap, pos & PAGE_MASK); pgoff_t pgoff; long rc, id; void *kaddr; bool page_aligned = false; - + unsigned offset = offset_in_page(pos); + unsigned size = min_t(u64, PAGE_SIZE - offset, length); if (IS_ALIGNED(sector << SECTOR_SHIFT, PAGE_SIZE) && - IS_ALIGNED(size, PAGE_SIZE)) + (size == PAGE_SIZE)) page_aligned = true; rc = bdev_dax_pgoff(iomap->bdev, sector, PAGE_SIZE, &pgoff); @@ -1058,8 +1058,7 @@ int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, id = dax_read_lock(); if (page_aligned) - rc = dax_zero_page_range(iomap->dax_dev, pgoff, - size >> PAGE_SHIFT); + rc = dax_zero_page_range(iomap->dax_dev, pgoff, 1); else rc = dax_direct_access(iomap->dax_dev, pgoff, 1, &kaddr, NULL); if (rc < 0) { @@ -1072,7 +1071,7 @@ int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, dax_flush(iomap->dax_dev, kaddr + offset, size); } dax_read_unlock(id); - return 0; + return size; } static loff_t diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 7f618ab4b11e..2dba054095e8 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -901,11 +901,13 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, } EXPORT_SYMBOL_GPL(iomap_file_unshare); -static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, - unsigned bytes, struct iomap *iomap, struct iomap *srcmap) +static loff_t iomap_zero(struct inode *inode, loff_t pos, u64 length, + struct iomap *iomap, struct iomap *srcmap) { struct page *page; int status; + unsigned offset = offset_in_page(pos); + unsigned bytes = min_t(u64, PAGE_SIZE - offset, length); status = iomap_write_begin(inode, pos, bytes, 0, &page, iomap, srcmap); if (status) @@ -917,38 +919,33 @@ static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, return iomap_write_end(inode, pos, bytes, bytes, page, iomap, srcmap); } -static loff_t -iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, - void *data, struct iomap *iomap, struct iomap *srcmap) +static loff_t iomap_zero_range_actor(struct inode *inode, loff_t pos, + loff_t length, void *data, struct iomap *iomap, + struct iomap *srcmap) { bool *did_zero = data; loff_t written = 0; - int status; /* already zeroed? we're done. */ if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) - return count; + return length; do { - unsigned offset, bytes; - - offset = offset_in_page(pos); - bytes = min_t(loff_t, PAGE_SIZE - offset, count); + loff_t bytes; if (IS_DAX(inode)) - status = dax_iomap_zero(pos, offset, bytes, iomap); + bytes = dax_iomap_zero(pos, length, iomap); else - status = iomap_zero(inode, pos, offset, bytes, iomap, - srcmap); - if (status < 0) - return status; + bytes = iomap_zero(inode, pos, length, iomap, srcmap); + if (bytes < 0) + return bytes; pos += bytes; - count -= bytes; + length -= bytes; written += bytes; if (did_zero) *did_zero = true; - } while (count > 0); + } while (length > 0); return written; } diff --git a/include/linux/dax.h b/include/linux/dax.h index 6904d4e0b2e0..80f17946f940 100644 --- a/include/linux/dax.h +++ b/include/linux/dax.h @@ -214,8 +214,7 @@ vm_fault_t dax_finish_sync_fault(struct vm_fault *vmf, int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index); int dax_invalidate_mapping_entry_sync(struct address_space *mapping, pgoff_t index); -int dax_iomap_zero(loff_t pos, unsigned offset, unsigned size, - struct iomap *iomap); +loff_t dax_iomap_zero(loff_t pos, u64 length, struct iomap *iomap); static inline bool dax_mapping(struct address_space *mapping) { return mapping->host && IS_DAX(mapping->host);
Pass the full length to iomap_zero() and dax_iomap_zero(), and have them return how many bytes they actually handled. This is preparatory work for handling THP, although it looks like DAX could actually take advantage of it if there's a larger contiguous area. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/dax.c | 13 ++++++------- fs/iomap/buffered-io.c | 33 +++++++++++++++------------------ include/linux/dax.h | 3 +-- 3 files changed, 22 insertions(+), 27 deletions(-)