From: Yue Hu <[email protected]>
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 <[email protected]>
---
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;
}
--
2.17.1
On 2023/4/17 14:41, Yue Hu wrote:
> From: Yue Hu <[email protected]>
>
> 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 <[email protected]>
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;
> }
>
On 2023/4/17 14:52, Gao Xiang wrote:
>
>
> On 2023/4/17 14:41, Yue Hu wrote:
>> From: Yue Hu <[email protected]>
>>
>> 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 <[email protected]>
>
> 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 <[email protected]>
>
> 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;
>> }
On Mon, 17 Apr 2023 15:00:25 +0800
Gao Xiang <[email protected]> wrote:
> On 2023/4/17 14:52, Gao Xiang wrote:
> >
> >
> > On 2023/4/17 14:41, Yue Hu wrote:
> >> From: Yue Hu <[email protected]>
> >>
> >> 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 <[email protected]>
> >
> > 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 <[email protected]>
>
> >
> > Thanks,
> > Gao Xiang
> >
> >> ---
> >> ?0?2 fs/erofs/zdata.c | 13 +++++--------
> >> ?0?2 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 {
> >> ?0?2?0?2?0?2?0?2?0?2 /* used for applying cache strategy on the fly */
> >> ?0?2?0?2?0?2?0?2?0?2 bool backmost;
> >> ?0?2?0?2?0?2?0?2?0?2 erofs_off_t headoffset;
> >> -
> >> -?0?2?0?2?0?2 /* a pointer used to pick up inplace I/O pages */
> >> -?0?2?0?2?0?2 unsigned int icur;
> >> ?0?2 };
> >> ?0?2 #define DECOMPRESS_FRONTEND_INIT(__i) { \
> >> @@ -707,11 +704,13 @@ static bool z_erofs_try_inplace_io(struct z_erofs_decompress_frontend *fe,
> >> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 struct z_erofs_bvec *bvec)
> >> ?0?2 {
> >> ?0?2?0?2?0?2?0?2?0?2 struct z_erofs_pcluster *const pcl = fe->pcl;
> >> +?0?2?0?2?0?2 /* 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
>
> >> +?0?2?0?2?0?2 unsigned int icur = pcl->pclusterpages;
> >> -?0?2?0?2?0?2 while (fe->icur > 0) {
> >> -?0?2?0?2?0?2?0?2?0?2?0?2?0?2 if (!cmpxchg(&pcl->compressed_bvecs[--fe->icur].page,
> >> +?0?2?0?2?0?2 while (icur > 0) {
> >> +?0?2?0?2?0?2?0?2?0?2?0?2?0?2 if (!cmpxchg(&pcl->compressed_bvecs[--icur].page,
> >> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 NULL, bvec->page)) {
> >> -?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 pcl->compressed_bvecs[fe->icur] = *bvec;
> >> +?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 pcl->compressed_bvecs[icur] = *bvec;
> >> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 return true;
> >> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 }
> >> ?0?2?0?2?0?2?0?2?0?2 }
> >> @@ -877,8 +876,6 @@ static int z_erofs_collector_begin(struct z_erofs_decompress_frontend *fe)
> >> ?0?2?0?2?0?2?0?2?0?2 }
> >> ?0?2?0?2?0?2?0?2?0?2 z_erofs_bvec_iter_begin(&fe->biter, &fe->pcl->bvset,
> >> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 Z_EROFS_INLINE_BVECS, fe->pcl->vcnt);
> >> -?0?2?0?2?0?2 /* since file-backed online pages are traversed in reverse order */
> >> -?0?2?0?2?0?2 fe->icur = z_erofs_pclusterpages(fe->pcl);
> >> ?0?2?0?2?0?2?0?2?0?2 return 0;
> >> ?0?2 }
On 2023/4/17 15:15, Yue Hu wrote:
> On Mon, 17 Apr 2023 15:00:25 +0800
> Gao Xiang <[email protected]> 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;
>>>> }
On Mon, 17 Apr 2023 15:12:31 +0800
Gao Xiang <[email protected]> wrote:
> On 2023/4/17 15:15, Yue Hu wrote:
> > On Mon, 17 Apr 2023 15:00:25 +0800
> > Gao Xiang <[email protected]> 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
> >>
> >>>> +?0?2?0?2?0?2 unsigned int icur = pcl->pclusterpages;
> >>>> -?0?2?0?2?0?2 while (fe->icur > 0) {
> >>>> -?0?2?0?2?0?2?0?2?0?2?0?2?0?2 if (!cmpxchg(&pcl->compressed_bvecs[--fe->icur].page,
> >>>> +?0?2?0?2?0?2 while (icur > 0) {
> >>>> +?0?2?0?2?0?2?0?2?0?2?0?2?0?2 if (!cmpxchg(&pcl->compressed_bvecs[--icur].page,
> >>>> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 NULL, bvec->page)) {
> >>>> -?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 pcl->compressed_bvecs[fe->icur] = *bvec;
> >>>> +?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 pcl->compressed_bvecs[icur] = *bvec;
> >>>> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 return true;
> >>>> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 }
> >>>> ?0?2?0?2?0?2?0?2?0?2 }
> >>>> @@ -877,8 +876,6 @@ static int z_erofs_collector_begin(struct z_erofs_decompress_frontend *fe)
> >>>> ?0?2?0?2?0?2?0?2?0?2 }
> >>>> ?0?2?0?2?0?2?0?2?0?2 z_erofs_bvec_iter_begin(&fe->biter, &fe->pcl->bvset,
> >>>> ?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2?0?2 Z_EROFS_INLINE_BVECS, fe->pcl->vcnt);
> >>>> -?0?2?0?2?0?2 /* since file-backed online pages are traversed in reverse order */
> >>>> -?0?2?0?2?0?2 fe->icur = z_erofs_pclusterpages(fe->pcl);
> >>>> ?0?2?0?2?0?2?0?2?0?2 return 0;
> >>>> ?0?2 }