diff mbox series

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

Message ID 20230918110510.66470-8-hare@suse.de (mailing list archive)
State New, archived
Headers show
Series block: update buffer_head for Large-block I/O | expand

Commit Message

Hannes Reinecke Sept. 18, 2023, 11:04 a.m. UTC 
Use mapping_min_folio_order() when calling filemap_alloc_folio()
to allocate folios with the order specified by the mapping.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 mm/filemap.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Matthew Wilcox Sept. 18, 2023, 1:41 p.m. UTC | #1 
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.
Hannes Reinecke Sept. 18, 2023, 5:34 p.m. UTC | #2 
On 9/18/23 15:41, Matthew Wilcox wrote:
> 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.
> 
Because 'end' is the _last_ valid index, not the index at which the 
iteration stops (cf 'index <= end') here. And as the index remains in 4k 
units we need to align both, index and end, to the nearest folio.

>> @@ -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?
> 
Surely.

>> @@ -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'.
> 
Does it? Cool. Then of course we don't need to align the index here.

>>   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.
> 
I can check if that makes a difference.

>> @@ -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.

Why, but of course. Will check.

Cheers,

Hannes
diff mbox series

Patch

diff --git a/mm/filemap.c b/mm/filemap.c
index 582f5317ff71..98c3737644d5 100644
--- a/mm/filemap.c
+++ 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;
 
@@ -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;
 
@@ -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);
+	}
 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);
 		if (err == AOP_TRUNCATED_PAGE)
 			goto retry;
 		return err;
@@ -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);