diff mbox series

erofs: remove unneeded icur field from struct z_erofs_decompress_frontend

Message ID 20230417064136.5890-1-zbestahu@gmail.com
State New
Headers
Series erofs: remove unneeded icur field from struct z_erofs_decompress_frontend |

Commit Message

Yue Hu April 17, 2023, 6:41 a.m. UTC 
  From: Yue Hu <huyue2@coolpad.com>

The icur field is only used in z_erofs_try_inplace_io(). Let's just use
a local variable instead. And no need to check if the pcluster is inline
when setting icur since inline page cannot be used for inplace I/O.

Signed-off-by: Yue Hu <huyue2@coolpad.com>
---
 fs/erofs/zdata.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Comments

Gao Xiang April 17, 2023, 6:52 a.m. UTC | #1 
On 2023/4/17 14:41, Yue Hu wrote:
> From: Yue Hu <huyue2@coolpad.com>
> 
> The icur field is only used in z_erofs_try_inplace_io(). Let's just use
> a local variable instead. And no need to check if the pcluster is inline
> when setting icur since inline page cannot be used for inplace I/O.
> 
> Signed-off-by: Yue Hu <huyue2@coolpad.com>

Nope, it's a behavior change.
Other users could feed more inplace I/O pages before I/O submission
to reduce memory consumption, it's common when applying stress model
in low memory scenarios.

Thanks,
Gao Xiang

> ---
>   fs/erofs/zdata.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index f759152feffa..f8bf2b227942 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -554,9 +554,6 @@ struct z_erofs_decompress_frontend {
>   	/* used for applying cache strategy on the fly */
>   	bool backmost;
>   	erofs_off_t headoffset;
> -
> -	/* a pointer used to pick up inplace I/O pages */
> -	unsigned int icur;
>   };
>   
>   #define DECOMPRESS_FRONTEND_INIT(__i) { \
> @@ -707,11 +704,13 @@ static bool z_erofs_try_inplace_io(struct z_erofs_decompress_frontend *fe,
>   				   struct z_erofs_bvec *bvec)
>   {
>   	struct z_erofs_pcluster *const pcl = fe->pcl;
> +	/* file-backed online pages are traversed in reverse order */
> +	unsigned int icur = pcl->pclusterpages;
>   
> -	while (fe->icur > 0) {
> -		if (!cmpxchg(&pcl->compressed_bvecs[--fe->icur].page,
> +	while (icur > 0) {
> +		if (!cmpxchg(&pcl->compressed_bvecs[--icur].page,
>   			     NULL, bvec->page)) {
> -			pcl->compressed_bvecs[fe->icur] = *bvec;
> +			pcl->compressed_bvecs[icur] = *bvec;
>   			return true;
>   		}
>   	}
> @@ -877,8 +876,6 @@ static int z_erofs_collector_begin(struct z_erofs_decompress_frontend *fe)
>   	}
>   	z_erofs_bvec_iter_begin(&fe->biter, &fe->pcl->bvset,
>   				Z_EROFS_INLINE_BVECS, fe->pcl->vcnt);
> -	/* since file-backed online pages are traversed in reverse order */
> -	fe->icur = z_erofs_pclusterpages(fe->pcl);
>   	return 0;
>   }
>
Gao Xiang April 17, 2023, 7 a.m. UTC | #2 
On 2023/4/17 14:52, Gao Xiang wrote:
> 
> 
> On 2023/4/17 14:41, Yue Hu wrote:
>> From: Yue Hu <huyue2@coolpad.com>
>>
>> The icur field is only used in z_erofs_try_inplace_io(). Let's just use
>> a local variable instead. And no need to check if the pcluster is inline
>> when setting icur since inline page cannot be used for inplace I/O.

Where do we check if the pcluster is inline?

>>
>> Signed-off-by: Yue Hu <huyue2@coolpad.com>
> 
> Nope, it's a behavior change.
> Other users could feed more inplace I/O pages before I/O submission
> to reduce memory consumption, it's common when applying stress model
> in low memory scenarios.

Oh, I misread it.  I think it can be done in this way although
each following users will now rescan the whole array all the time.

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

> 
> Thanks,
> Gao Xiang
> 
>> ---
>>   fs/erofs/zdata.c | 13 +++++--------
>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
>> index f759152feffa..f8bf2b227942 100644
>> --- a/fs/erofs/zdata.c
>> +++ b/fs/erofs/zdata.c
>> @@ -554,9 +554,6 @@ struct z_erofs_decompress_frontend {
>>       /* used for applying cache strategy on the fly */
>>       bool backmost;
>>       erofs_off_t headoffset;
>> -
>> -    /* a pointer used to pick up inplace I/O pages */
>> -    unsigned int icur;
>>   };
>>   #define DECOMPRESS_FRONTEND_INIT(__i) { \
>> @@ -707,11 +704,13 @@ static bool z_erofs_try_inplace_io(struct z_erofs_decompress_frontend *fe,
>>                      struct z_erofs_bvec *bvec)
>>   {
>>       struct z_erofs_pcluster *const pcl = fe->pcl;
>> +    /* file-backed online pages are traversed in reverse order */

Although please help refine the comment below:

	/* scan & fill inplace I/O pages in the reverse order */

Thanks,
Gao Xiang

>> +    unsigned int icur = pcl->pclusterpages;
>> -    while (fe->icur > 0) {
>> -        if (!cmpxchg(&pcl->compressed_bvecs[--fe->icur].page,
>> +    while (icur > 0) {
>> +        if (!cmpxchg(&pcl->compressed_bvecs[--icur].page,
>>                    NULL, bvec->page)) {
>> -            pcl->compressed_bvecs[fe->icur] = *bvec;
>> +            pcl->compressed_bvecs[icur] = *bvec;
>>               return true;
>>           }
>>       }
>> @@ -877,8 +876,6 @@ static int z_erofs_collector_begin(struct z_erofs_decompress_frontend *fe)
>>       }
>>       z_erofs_bvec_iter_begin(&fe->biter, &fe->pcl->bvset,
>>                   Z_EROFS_INLINE_BVECS, fe->pcl->vcnt);
>> -    /* since file-backed online pages are traversed in reverse order */
>> -    fe->icur = z_erofs_pclusterpages(fe->pcl);
>>       return 0;
>>   }
Gao Xiang April 17, 2023, 7:12 a.m. UTC | #3 
On 2023/4/17 15:15, Yue Hu wrote:
> On Mon, 17 Apr 2023 15:00:25 +0800
> Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> 

..

>>
>> Although please help refine the comment below:
>>
>> 	/* scan & fill inplace I/O pages in the reverse order */
> 
> Ok, will refine it in v2.

I rethink this, I don't want to go far in this way, and this makes a
O(n) scan into O(n^2) when a single inplace I/O page is added.

So sorry, I don't think it's a good way, although I also don't think
`icur` is a good name and we might need to find a better name.

Thanks,
Gao Xiang

> 
>>
>> Thanks,
>> Gao Xiang
>>
>>>> +    unsigned int icur = pcl->pclusterpages;
>>>> -    while (fe->icur > 0) {
>>>> -        if (!cmpxchg(&pcl->compressed_bvecs[--fe->icur].page,
>>>> +    while (icur > 0) {
>>>> +        if (!cmpxchg(&pcl->compressed_bvecs[--icur].page,
>>>>                     NULL, bvec->page)) {
>>>> -            pcl->compressed_bvecs[fe->icur] = *bvec;
>>>> +            pcl->compressed_bvecs[icur] = *bvec;
>>>>                return true;
>>>>            }
>>>>        }
>>>> @@ -877,8 +876,6 @@ static int z_erofs_collector_begin(struct z_erofs_decompress_frontend *fe)
>>>>        }
>>>>        z_erofs_bvec_iter_begin(&fe->biter, &fe->pcl->bvset,
>>>>                    Z_EROFS_INLINE_BVECS, fe->pcl->vcnt);
>>>> -    /* since file-backed online pages are traversed in reverse order */
>>>> -    fe->icur = z_erofs_pclusterpages(fe->pcl);
>>>>        return 0;
>>>>    }
Yue Hu April 17, 2023, 7:15 a.m. UTC | #4 
On Mon, 17 Apr 2023 15:00:25 +0800
Gao Xiang <hsiangkao@linux.alibaba.com> wrote:

> On 2023/4/17 14:52, Gao Xiang wrote:
> > 
> > 
> > On 2023/4/17 14:41, Yue Hu wrote:  
> >> From: Yue Hu <huyue2@coolpad.com>
> >>
> >> The icur field is only used in z_erofs_try_inplace_io(). Let's just use
> >> a local variable instead. And no need to check if the pcluster is inline
> >> when setting icur since inline page cannot be used for inplace I/O.  
> 
> Where do we check if the pcluster is inline?

       /* since file-backed online pages are traversed in reverse order */
       fe->icur = z_erofs_pclusterpages(fe->pcl);


static inline unsigned int z_erofs_pclusterpages(struct z_erofs_pcluster *pcl)
{
        if (z_erofs_is_inline_pcluster(pcl))
                return 1;
	[...]

> 
> >>
> >> Signed-off-by: Yue Hu <huyue2@coolpad.com>  
> > 
> > Nope, it's a behavior change.
> > Other users could feed more inplace I/O pages before I/O submission
> > to reduce memory consumption, it's common when applying stress model
> > in low memory scenarios.  
> 
> Oh, I misread it.  I think it can be done in this way although
> each following users will now rescan the whole array all the time.
> 
> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> 
> > 
> > Thanks,
> > Gao Xiang
> >   
> >> ---
> >>   fs/erofs/zdata.c | 13 +++++--------
> >>   1 file changed, 5 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> >> index f759152feffa..f8bf2b227942 100644
> >> --- a/fs/erofs/zdata.c
> >> +++ b/fs/erofs/zdata.c
> >> @@ -554,9 +554,6 @@ struct z_erofs_decompress_frontend {
> >>       /* used for applying cache strategy on the fly */
> >>       bool backmost;
> >>       erofs_off_t headoffset;
> >> -
> >> -    /* a pointer used to pick up inplace I/O pages */
> >> -    unsigned int icur;
> >>   };
> >>   #define DECOMPRESS_FRONTEND_INIT(__i) { \
> >> @@ -707,11 +704,13 @@ static bool z_erofs_try_inplace_io(struct z_erofs_decompress_frontend *fe,
> >>                      struct z_erofs_bvec *bvec)
> >>   {
> >>       struct z_erofs_pcluster *const pcl = fe->pcl;
> >> +    /* file-backed online pages are traversed in reverse order */  
> 
> Although please help refine the comment below:
> 
> 	/* scan & fill inplace I/O pages in the reverse order */

Ok, will refine it in v2.

> 
> Thanks,
> Gao Xiang
> 
> >> +    unsigned int icur = pcl->pclusterpages;
> >> -    while (fe->icur > 0) {
> >> -        if (!cmpxchg(&pcl->compressed_bvecs[--fe->icur].page,
> >> +    while (icur > 0) {
> >> +        if (!cmpxchg(&pcl->compressed_bvecs[--icur].page,
> >>                    NULL, bvec->page)) {
> >> -            pcl->compressed_bvecs[fe->icur] = *bvec;
> >> +            pcl->compressed_bvecs[icur] = *bvec;
> >>               return true;
> >>           }
> >>       }
> >> @@ -877,8 +876,6 @@ static int z_erofs_collector_begin(struct z_erofs_decompress_frontend *fe)
> >>       }
> >>       z_erofs_bvec_iter_begin(&fe->biter, &fe->pcl->bvset,
> >>                   Z_EROFS_INLINE_BVECS, fe->pcl->vcnt);
> >> -    /* since file-backed online pages are traversed in reverse order */
> >> -    fe->icur = z_erofs_pclusterpages(fe->pcl);
> >>       return 0;
> >>   }
Yue Hu April 17, 2023, 7:44 a.m. UTC | #5 
On Mon, 17 Apr 2023 15:12:31 +0800
Gao Xiang <hsiangkao@linux.alibaba.com> wrote:

> On 2023/4/17 15:15, Yue Hu wrote:
> > On Mon, 17 Apr 2023 15:00:25 +0800
> > Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >   
> 
> ..
> 
> >>
> >> Although please help refine the comment below:
> >>
> >> 	/* scan & fill inplace I/O pages in the reverse order */  
> > 
> > Ok, will refine it in v2.  
> 
> I rethink this, I don't want to go far in this way, and this makes a
> O(n) scan into O(n^2) when a single inplace I/O page is added.

Yeah, i misread that, it should be global before submission, just ignore the change.

> 
> So sorry, I don't think it's a good way, although I also don't think
> `icur` is a good name and we might need to find a better name.
> 
> Thanks,
> Gao Xiang
> 
> >   
> >>
> >> Thanks,
> >> Gao Xiang
> >>  
> >>>> +    unsigned int icur = pcl->pclusterpages;
> >>>> -    while (fe->icur > 0) {
> >>>> -        if (!cmpxchg(&pcl->compressed_bvecs[--fe->icur].page,
> >>>> +    while (icur > 0) {
> >>>> +        if (!cmpxchg(&pcl->compressed_bvecs[--icur].page,
> >>>>                     NULL, bvec->page)) {
> >>>> -            pcl->compressed_bvecs[fe->icur] = *bvec;
> >>>> +            pcl->compressed_bvecs[icur] = *bvec;
> >>>>                return true;
> >>>>            }
> >>>>        }
> >>>> @@ -877,8 +876,6 @@ static int z_erofs_collector_begin(struct z_erofs_decompress_frontend *fe)
> >>>>        }
> >>>>        z_erofs_bvec_iter_begin(&fe->biter, &fe->pcl->bvset,
> >>>>                    Z_EROFS_INLINE_BVECS, fe->pcl->vcnt);
> >>>> -    /* since file-backed online pages are traversed in reverse order */
> >>>> -    fe->icur = z_erofs_pclusterpages(fe->pcl);
> >>>>        return 0;
> >>>>    }
diff mbox series

Patch

diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index f759152feffa..f8bf2b227942 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -554,9 +554,6 @@  struct z_erofs_decompress_frontend {
 	/* used for applying cache strategy on the fly */
 	bool backmost;
 	erofs_off_t headoffset;
-
-	/* a pointer used to pick up inplace I/O pages */
-	unsigned int icur;
 };
 
 #define DECOMPRESS_FRONTEND_INIT(__i) { \
@@ -707,11 +704,13 @@  static bool z_erofs_try_inplace_io(struct z_erofs_decompress_frontend *fe,
 				   struct z_erofs_bvec *bvec)
 {
 	struct z_erofs_pcluster *const pcl = fe->pcl;
+	/* file-backed online pages are traversed in reverse order */
+	unsigned int icur = pcl->pclusterpages;
 
-	while (fe->icur > 0) {
-		if (!cmpxchg(&pcl->compressed_bvecs[--fe->icur].page,
+	while (icur > 0) {
+		if (!cmpxchg(&pcl->compressed_bvecs[--icur].page,
 			     NULL, bvec->page)) {
-			pcl->compressed_bvecs[fe->icur] = *bvec;
+			pcl->compressed_bvecs[icur] = *bvec;
 			return true;
 		}
 	}
@@ -877,8 +876,6 @@  static int z_erofs_collector_begin(struct z_erofs_decompress_frontend *fe)
 	}
 	z_erofs_bvec_iter_begin(&fe->biter, &fe->pcl->bvset,
 				Z_EROFS_INLINE_BVECS, fe->pcl->vcnt);
-	/* since file-backed online pages are traversed in reverse order */
-	fe->icur = z_erofs_pclusterpages(fe->pcl);
 	return 0;
 }