Re: [PATCH v3] iomap: avoid redundant fault_in_iov_iter_readable() judgement when use larger chunks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, May 20, 2024 at 08:08:23AM -0700, Darrick J. Wong wrote:
> On Mon, May 20, 2024 at 06:55:25PM +0800, Xu Yang wrote:
> > Since commit (5d8edfb900d5 "iomap: Copy larger chunks from userspace"),
> > iomap will try to copy in larger chunks than PAGE_SIZE. However, if the
> > mapping doesn't support large folio, only one page of maximum 4KB will
> > be created and 4KB data will be writen to pagecache each time. Then,
> > next 4KB will be handled in next iteration. This will cause potential
> > write performance problem.
> > 
> > If chunk is 2MB, total 512 pages need to be handled finally. During this
> > period, fault_in_iov_iter_readable() is called to check iov_iter readable
> > validity. Since only 4KB will be handled each time, below address space
> > will be checked over and over again:
> > 
> > start         	end
> > -
> > buf,    	buf+2MB
> > buf+4KB, 	buf+2MB
> > buf+8KB, 	buf+2MB
> > ...
> > buf+2044KB 	buf+2MB
> > 
> > Obviously the checking size is wrong since only 4KB will be handled each
> > time. So this will get a correct chunk to let iomap work well in non-large
> > folio case.
> > 
> > With this change, the write speed will be stable. Tested on ARM64 device.
> > 
> > Before:
> > 
> >  - dd if=/dev/zero of=/dev/sda bs=400K  count=10485  (334 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=800K  count=5242   (278 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=1600K count=2621   (204 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=2200K count=1906   (170 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=3000K count=1398   (150 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=4500K count=932    (139 MB/s)
> > 
> > After:
> > 
> >  - dd if=/dev/zero of=/dev/sda bs=400K  count=10485  (339 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=800K  count=5242   (330 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=1600K count=2621   (332 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=2200K count=1906   (333 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=3000K count=1398   (333 MB/s)
> >  - dd if=/dev/zero of=/dev/sda bs=4500K count=932    (333 MB/s)
> > 
> > Fixes: 5d8edfb900d5 ("iomap: Copy larger chunks from userspace")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> > 
> > ---
> > Changes in v2:
> >  - fix address space description in message
> > Changes in v3:
> >  - adjust 'chunk' and add mapping_max_folio_size() in header file
> >    as suggested by Matthew
> >  - add write performance results in commit message
> > ---
> >  fs/iomap/buffered-io.c  |  2 +-
> >  include/linux/pagemap.h | 37 ++++++++++++++++++++++++-------------
> >  2 files changed, 25 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 41c8f0c68ef5..c5802a459334 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -898,11 +898,11 @@ static bool iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
> >  static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
> >  {
> >  	loff_t length = iomap_length(iter);
> > -	size_t chunk = PAGE_SIZE << MAX_PAGECACHE_ORDER;
> >  	loff_t pos = iter->pos;
> >  	ssize_t total_written = 0;
> >  	long status = 0;
> >  	struct address_space *mapping = iter->inode->i_mapping;
> > +	size_t chunk = mapping_max_folio_size(mapping);
> >  	unsigned int bdp_flags = (iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
> >  
> >  	do {
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index c5e33e2ca48a..6be8e22360f1 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -346,6 +346,19 @@ static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask)
> >  	m->gfp_mask = mask;
> >  }
> >  
> > +/*
> > + * There are some parts of the kernel which assume that PMD entries
> > + * are exactly HPAGE_PMD_ORDER.  Those should be fixed, but until then,
> > + * limit the maximum allocation order to PMD size.  I'm not aware of any
> > + * assumptions about maximum order if THP are disabled, but 8 seems like
> > + * a good order (that's 1MB if you're using 4kB pages)
> > + */
> > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > +#define MAX_PAGECACHE_ORDER	HPAGE_PMD_ORDER
> > +#else
> > +#define MAX_PAGECACHE_ORDER	8
> > +#endif
> > +
> >  /**
> >   * mapping_set_large_folios() - Indicate the file supports large folios.
> >   * @mapping: The file.
> > @@ -372,6 +385,17 @@ static inline bool mapping_large_folio_support(struct address_space *mapping)
> >  		test_bit(AS_LARGE_FOLIO_SUPPORT, &mapping->flags);
> >  }
> >  
> > +/*
> > + * Get max folio size in case of supporting large folio, otherwise return
> > + * PAGE_SIZE.
> 
> Minor quibble -- the comment doesn't need to restate what the function
> does because we can see that in the code below.
> 
> /* Return the maximum folio size for this pagecache mapping, in bytes. */
> 
> With that fixed,
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> 

Okay, will change the comment in v4.

Thanks,
Xu Yang

> --D
> 
> 
> > + */
> > +static inline size_t mapping_max_folio_size(struct address_space *mapping)
> > +{
> > +	if (mapping_large_folio_support(mapping))
> > +		return PAGE_SIZE << MAX_PAGECACHE_ORDER;
> > +	return PAGE_SIZE;
> > +}
> > +
> >  static inline int filemap_nr_thps(struct address_space *mapping)
> >  {
> >  #ifdef CONFIG_READ_ONLY_THP_FOR_FS
> > @@ -530,19 +554,6 @@ static inline void *detach_page_private(struct page *page)
> >  	return folio_detach_private(page_folio(page));
> >  }
> >  
> > -/*
> > - * There are some parts of the kernel which assume that PMD entries
> > - * are exactly HPAGE_PMD_ORDER.  Those should be fixed, but until then,
> > - * limit the maximum allocation order to PMD size.  I'm not aware of any
> > - * assumptions about maximum order if THP are disabled, but 8 seems like
> > - * a good order (that's 1MB if you're using 4kB pages)
> > - */
> > -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > -#define MAX_PAGECACHE_ORDER	HPAGE_PMD_ORDER
> > -#else
> > -#define MAX_PAGECACHE_ORDER	8
> > -#endif
> > -
> >  #ifdef CONFIG_NUMA
> >  struct folio *filemap_alloc_folio(gfp_t gfp, unsigned int order);
> >  #else
> > -- 
> > 2.34.1
> > 
> > 




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux