Re: [PATCH 07/18] mm/filemap: allocate folios with mapping order preference

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

 



On Mon, Sep 18, 2023 at 01:04:59PM +0200, Hannes Reinecke wrote:
> +++ b/mm/filemap.c
> @@ -507,9 +507,14 @@ static void __filemap_fdatawait_range(struct address_space *mapping,
>  	pgoff_t end = end_byte >> PAGE_SHIFT;
>  	struct folio_batch fbatch;
>  	unsigned nr_folios;
> +	unsigned int order = mapping_min_folio_order(mapping);
>  
>  	folio_batch_init(&fbatch);
>  
> +	if (order) {
> +		index = ALIGN_DOWN(index, 1 << order);
> +		end = ALIGN_DOWN(end, 1 << order);
> +	}
>  	while (index <= end) {
>  		unsigned i;
>  

I don't understand why this function needs to change at all.
filemap_get_folios_tag() should return any folios which overlap
(index, end).  And aligning 'end' _down_ certainly sets off alarm bells
for me.  We surely would need to align _up_.  Except i don't think we
need to do anything to this function.

> @@ -2482,7 +2487,8 @@ static int filemap_create_folio(struct file *file,
>  	struct folio *folio;
>  	int error;
>  
> -	folio = filemap_alloc_folio(mapping_gfp_mask(mapping), 0);
> +	folio = filemap_alloc_folio(mapping_gfp_mask(mapping),
> +				    mapping_min_folio_order(mapping));
>  	if (!folio)
>  		return -ENOMEM;
>  

Surely we need to align 'index' here?

> @@ -2542,9 +2548,16 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
>  	pgoff_t last_index;
>  	struct folio *folio;
>  	int err = 0;
> +	unsigned int order = mapping_min_folio_order(mapping);
>  
>  	/* "last_index" is the index of the page beyond the end of the read */
>  	last_index = DIV_ROUND_UP(iocb->ki_pos + count, PAGE_SIZE);
> +	if (order) {
> +		/* Align with folio order */
> +		WARN_ON(index % 1 << order);
> +		index = ALIGN_DOWN(index, 1 << order);
> +		last_index = ALIGN(last_index, 1 << order);
> +	}

Not sure I see the point of this.  filemap_get_read_batch() returns any
folio which contains 'index'.

>  retry:
>  	if (fatal_signal_pending(current))
>  		return -EINTR;
> @@ -2561,7 +2574,7 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
>  		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
>  			return -EAGAIN;
>  		err = filemap_create_folio(filp, mapping,
> -				iocb->ki_pos >> PAGE_SHIFT, fbatch);
> +				index, fbatch);

... ah, you align index here.  I wonder if we wouldn't be better passing
iocb->ki_pos to filemap_create_folio() to emphasise that the caller
can't assume anything about the alignment/size of the folio.

> @@ -3676,7 +3689,8 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
>  repeat:
>  	folio = filemap_get_folio(mapping, index);
>  	if (IS_ERR(folio)) {
> -		folio = filemap_alloc_folio(gfp, 0);
> +		folio = filemap_alloc_folio(gfp,
> +				mapping_min_folio_order(mapping));
>  		if (!folio)
>  			return ERR_PTR(-ENOMEM);
>  		err = filemap_add_folio(mapping, folio, index, gfp);

This needs to align index.



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux