[PREVIEW] [PATCH 1/4] staging: erofs: remove the unneeded marco in xattr submodule

[PREVIEW] [PATCH 1/4] staging: erofs: remove the unneeded marco in xattr submodule

[Gao Xiang]

There is no need to '#if CONFIG_EROFS_FS_XATTR' in xattr.c,
let's remove it.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/xattr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index b74b314..6b9685f 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -420,7 +420,6 @@ static int erofs_xattr_generic_get(const struct xattr_handler *handler,
 };
 #endif
 
-#ifdef CONFIG_EROFS_FS_XATTR
 const struct xattr_handler *erofs_xattr_handlers[] = {
 	&erofs_xattr_user_handler,
 #ifdef CONFIG_EROFS_FS_POSIX_ACL
@@ -433,7 +432,6 @@ static int erofs_xattr_generic_get(const struct xattr_handler *handler,
 #endif
 	NULL,
 };
-#endif
 
 struct listxattr_iter {
 	struct xattr_iter it;
-- 
1.9.1

[PREVIEW] [PATCH 2/4] staging: erofs: introduce erofs_grab_bio

[Gao Xiang]

this patch renames prepare_bio to erofs_grab_bio, and
adds a nofail option in order to retry in the bio allocator.

Reported-by: Stephen Rothwell <sfr at canb.auug.org.au>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/data.c      | 12 ++++++++++--
 drivers/staging/erofs/internal.h  | 35 +++++++++++++++++------------------
 drivers/staging/erofs/unzip_vle.c |  4 ++--
 3 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index ac263a1..2426eda 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -60,7 +60,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
 		struct bio *bio;
 		int err;
 
-		bio = prepare_bio(sb, blkaddr, 1, read_endio);
+		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
+
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
 		BUG_ON(err != PAGE_SIZE);
 
@@ -278,7 +279,14 @@ static inline struct bio *erofs_read_raw_page(
 		if (nblocks > BIO_MAX_PAGES)
 			nblocks = BIO_MAX_PAGES;
 
-		bio = prepare_bio(inode->i_sb, blknr, nblocks, read_endio);
+		bio = erofs_grab_bio(inode->i_sb,
+			blknr, nblocks, read_endio, false);
+
+		if (unlikely(IS_ERR(bio))) {
+			err = PTR_ERR(bio);
+			bio = NULL;
+			goto err_out;
+		}
 	}
 
 	err = bio_add_page(bio, page, PAGE_SIZE, 0);
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 4518729..acce2d6 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -419,26 +419,25 @@ struct erofs_map_blocks {
 #define EROFS_GET_BLOCKS_RAW    0x0001
 
 /* data.c */
-static inline struct bio *prepare_bio(
-	struct super_block *sb,
-	erofs_blk_t blkaddr, unsigned nr_pages,
-	bio_end_io_t endio)
+static inline struct bio *
+erofs_grab_bio(struct super_block *sb,
+	       erofs_blk_t blkaddr, unsigned nr_pages,
+	       bio_end_io_t endio, bool nofail)
 {
-	gfp_t gfp = GFP_NOIO;
-	struct bio *bio = bio_alloc(gfp, nr_pages);
-
-	if (unlikely(bio == NULL) &&
-		(current->flags & PF_MEMALLOC)) {
-		do {
-			nr_pages /= 2;
-			if (unlikely(!nr_pages)) {
-				bio = bio_alloc(gfp | __GFP_NOFAIL, 1);
-				BUG_ON(bio == NULL);
-				break;
+	const gfp_t gfp = GFP_NOIO;
+	struct bio *bio;
+
+	do {
+		if (nr_pages == 1) {
+			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
+			if (unlikely(bio == NULL)) {
+				DBG_BUGON(nofail);
+				return ERR_PTR(-ENOMEM);
 			}
-			bio = bio_alloc(gfp, nr_pages);
-		} while (bio == NULL);
-	}
+		}
+		bio = bio_alloc(gfp, nr_pages);
+		nr_pages /= 2;
+	} while (unlikely(bio == NULL));
 
 	bio->bi_end_io = endio;
 	bio_set_dev(bio, sb->s_bdev);
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 6d3ab31..be3929d 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1212,8 +1212,8 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
 		}
 
 		if (bio == NULL) {
-			bio = prepare_bio(sb, first_index + i,
-				BIO_MAX_PAGES, z_erofs_vle_read_endio);
+			bio = erofs_grab_bio(sb, first_index + i,
+				BIO_MAX_PAGES, z_erofs_vle_read_endio, true);
 			bio->bi_private = tagptr_cast_ptr(bi_private);
 
 			++nr_bios;
-- 
1.9.1

[PREVIEW] [PATCH RESEND 2/4] staging: erofs: introduce erofs_grab_bio

[Gao Xiang]

this patch renames prepare_bio to erofs_grab_bio, and
adds a nofail option in order to retry in the bio allocator.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/data.c      | 12 ++++++++++--
 drivers/staging/erofs/internal.h  | 35 +++++++++++++++++------------------
 drivers/staging/erofs/unzip_vle.c |  4 ++--
 3 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index ac263a1..2426eda 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -60,7 +60,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
 		struct bio *bio;
 		int err;
 
-		bio = prepare_bio(sb, blkaddr, 1, read_endio);
+		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
+
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
 		BUG_ON(err != PAGE_SIZE);
 
@@ -278,7 +279,14 @@ static inline struct bio *erofs_read_raw_page(
 		if (nblocks > BIO_MAX_PAGES)
 			nblocks = BIO_MAX_PAGES;
 
-		bio = prepare_bio(inode->i_sb, blknr, nblocks, read_endio);
+		bio = erofs_grab_bio(inode->i_sb,
+			blknr, nblocks, read_endio, false);
+
+		if (unlikely(IS_ERR(bio))) {
+			err = PTR_ERR(bio);
+			bio = NULL;
+			goto err_out;
+		}
 	}
 
 	err = bio_add_page(bio, page, PAGE_SIZE, 0);
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 4518729..acce2d6 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -419,26 +419,25 @@ struct erofs_map_blocks {
 #define EROFS_GET_BLOCKS_RAW    0x0001
 
 /* data.c */
-static inline struct bio *prepare_bio(
-	struct super_block *sb,
-	erofs_blk_t blkaddr, unsigned nr_pages,
-	bio_end_io_t endio)
+static inline struct bio *
+erofs_grab_bio(struct super_block *sb,
+	       erofs_blk_t blkaddr, unsigned nr_pages,
+	       bio_end_io_t endio, bool nofail)
 {
-	gfp_t gfp = GFP_NOIO;
-	struct bio *bio = bio_alloc(gfp, nr_pages);
-
-	if (unlikely(bio == NULL) &&
-		(current->flags & PF_MEMALLOC)) {
-		do {
-			nr_pages /= 2;
-			if (unlikely(!nr_pages)) {
-				bio = bio_alloc(gfp | __GFP_NOFAIL, 1);
-				BUG_ON(bio == NULL);
-				break;
+	const gfp_t gfp = GFP_NOIO;
+	struct bio *bio;
+
+	do {
+		if (nr_pages == 1) {
+			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
+			if (unlikely(bio == NULL)) {
+				DBG_BUGON(nofail);
+				return ERR_PTR(-ENOMEM);
 			}
-			bio = bio_alloc(gfp, nr_pages);
-		} while (bio == NULL);
-	}
+		}
+		bio = bio_alloc(gfp, nr_pages);
+		nr_pages /= 2;
+	} while (unlikely(bio == NULL));
 
 	bio->bi_end_io = endio;
 	bio_set_dev(bio, sb->s_bdev);
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 6d3ab31..be3929d 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1212,8 +1212,8 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
 		}
 
 		if (bio == NULL) {
-			bio = prepare_bio(sb, first_index + i,
-				BIO_MAX_PAGES, z_erofs_vle_read_endio);
+			bio = erofs_grab_bio(sb, first_index + i,
+				BIO_MAX_PAGES, z_erofs_vle_read_endio, true);
 			bio->bi_private = tagptr_cast_ptr(bi_private);
 
 			++nr_bios;
-- 
1.9.1

[PREVIEW] [PATCH RESEND 2/4] staging: erofs: introduce erofs_grab_bio

[Chao Yu]

On 2018/8/1 14:04, Gao Xiang wrote:
> this patch renames prepare_bio to erofs_grab_bio, and
> adds a nofail option in order to retry in the bio allocator.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
> ---
>  drivers/staging/erofs/data.c      | 12 ++++++++++--
>  drivers/staging/erofs/internal.h  | 35 +++++++++++++++++------------------
>  drivers/staging/erofs/unzip_vle.c |  4 ++--
>  3 files changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index ac263a1..2426eda 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -60,7 +60,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
>  		struct bio *bio;
>  		int err;
>  
> -		bio = prepare_bio(sb, blkaddr, 1, read_endio);
> +		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
> +
>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
>  		BUG_ON(err != PAGE_SIZE);
>  
> @@ -278,7 +279,14 @@ static inline struct bio *erofs_read_raw_page(
>  		if (nblocks > BIO_MAX_PAGES)
>  			nblocks = BIO_MAX_PAGES;
>  
> -		bio = prepare_bio(inode->i_sb, blknr, nblocks, read_endio);
> +		bio = erofs_grab_bio(inode->i_sb,
> +			blknr, nblocks, read_endio, false);
> +
> +		if (unlikely(IS_ERR(bio))) {
> +			err = PTR_ERR(bio);
> +			bio = NULL;
> +			goto err_out;
> +		}
>  	}
>  
>  	err = bio_add_page(bio, page, PAGE_SIZE, 0);
> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
> index 4518729..acce2d6 100644
> --- a/drivers/staging/erofs/internal.h
> +++ b/drivers/staging/erofs/internal.h
> @@ -419,26 +419,25 @@ struct erofs_map_blocks {
>  #define EROFS_GET_BLOCKS_RAW    0x0001
>  
>  /* data.c */
> -static inline struct bio *prepare_bio(
> -	struct super_block *sb,
> -	erofs_blk_t blkaddr, unsigned nr_pages,
> -	bio_end_io_t endio)
> +static inline struct bio *
> +erofs_grab_bio(struct super_block *sb,
> +	       erofs_blk_t blkaddr, unsigned nr_pages,
> +	       bio_end_io_t endio, bool nofail)
>  {
> -	gfp_t gfp = GFP_NOIO;
> -	struct bio *bio = bio_alloc(gfp, nr_pages);
> -
> -	if (unlikely(bio == NULL) &&
> -		(current->flags & PF_MEMALLOC)) {
> -		do {
> -			nr_pages /= 2;
> -			if (unlikely(!nr_pages)) {
> -				bio = bio_alloc(gfp | __GFP_NOFAIL, 1);
> -				BUG_ON(bio == NULL);
> -				break;
> +	const gfp_t gfp = GFP_NOIO;
> +	struct bio *bio;
> +
> +	do {
> +		if (nr_pages == 1) {
> +			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
> +			if (unlikely(bio == NULL)) {
> +				DBG_BUGON(nofail);
> +				return ERR_PTR(-ENOMEM);
>  			}
> -			bio = bio_alloc(gfp, nr_pages);
> -		} while (bio == NULL);
> -	}
> +		}
> +		bio = bio_alloc(gfp, nr_pages);

Need to cover this bio_alloc, so should initialize gfp as:

gfp_t gfp = GFP_NOIO | (nofail ? __GFP_NOFAIL : 0);

Thanks,

> +		nr_pages /= 2;
> +	} while (unlikely(bio == NULL));
>  
>  	bio->bi_end_io = endio;
>  	bio_set_dev(bio, sb->s_bdev);
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index 6d3ab31..be3929d 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -1212,8 +1212,8 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
>  		}
>  
>  		if (bio == NULL) {
> -			bio = prepare_bio(sb, first_index + i,
> -				BIO_MAX_PAGES, z_erofs_vle_read_endio);
> +			bio = erofs_grab_bio(sb, first_index + i,
> +				BIO_MAX_PAGES, z_erofs_vle_read_endio, true);
>  			bio->bi_private = tagptr_cast_ptr(bi_private);
>  
>  			++nr_bios;
> 

[PREVIEW] [PATCH RESEND 2/4] staging: erofs: introduce erofs_grab_bio

[Gao Xiang]

Hi Chao,

On 2018/8/10 10:46, Chao Yu wrote:
> On 2018/8/1 14:04, Gao Xiang wrote:
>> this patch renames prepare_bio to erofs_grab_bio, and
>> adds a nofail option in order to retry in the bio allocator.
>>
>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>> ---
>>  drivers/staging/erofs/data.c      | 12 ++++++++++--
>>  drivers/staging/erofs/internal.h  | 35 +++++++++++++++++------------------
>>  drivers/staging/erofs/unzip_vle.c |  4 ++--
>>  3 files changed, 29 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>> index ac263a1..2426eda 100644
>> --- a/drivers/staging/erofs/data.c
>> +++ b/drivers/staging/erofs/data.c
>> @@ -60,7 +60,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
>>  		struct bio *bio;
>>  		int err;
>>  
>> -		bio = prepare_bio(sb, blkaddr, 1, read_endio);
>> +		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
>> +
>>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
>>  		BUG_ON(err != PAGE_SIZE);
>>  
>> @@ -278,7 +279,14 @@ static inline struct bio *erofs_read_raw_page(
>>  		if (nblocks > BIO_MAX_PAGES)
>>  			nblocks = BIO_MAX_PAGES;
>>  
>> -		bio = prepare_bio(inode->i_sb, blknr, nblocks, read_endio);
>> +		bio = erofs_grab_bio(inode->i_sb,
>> +			blknr, nblocks, read_endio, false);
>> +
>> +		if (unlikely(IS_ERR(bio))) {
>> +			err = PTR_ERR(bio);
>> +			bio = NULL;
>> +			goto err_out;
>> +		}
>>  	}
>>  
>>  	err = bio_add_page(bio, page, PAGE_SIZE, 0);
>> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
>> index 4518729..acce2d6 100644
>> --- a/drivers/staging/erofs/internal.h
>> +++ b/drivers/staging/erofs/internal.h
>> @@ -419,26 +419,25 @@ struct erofs_map_blocks {
>>  #define EROFS_GET_BLOCKS_RAW    0x0001
>>  
>>  /* data.c */
>> -static inline struct bio *prepare_bio(
>> -	struct super_block *sb,
>> -	erofs_blk_t blkaddr, unsigned nr_pages,
>> -	bio_end_io_t endio)
>> +static inline struct bio *
>> +erofs_grab_bio(struct super_block *sb,
>> +	       erofs_blk_t blkaddr, unsigned nr_pages,
>> +	       bio_end_io_t endio, bool nofail)
>>  {
>> -	gfp_t gfp = GFP_NOIO;
>> -	struct bio *bio = bio_alloc(gfp, nr_pages);
>> -
>> -	if (unlikely(bio == NULL) &&
>> -		(current->flags & PF_MEMALLOC)) {
>> -		do {
>> -			nr_pages /= 2;
>> -			if (unlikely(!nr_pages)) {
>> -				bio = bio_alloc(gfp | __GFP_NOFAIL, 1);
>> -				BUG_ON(bio == NULL);
>> -				break;
>> +	const gfp_t gfp = GFP_NOIO;
>> +	struct bio *bio;
>> +
>> +	do {
>> +		if (nr_pages == 1) {
>> +			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
>> +			if (unlikely(bio == NULL)) {
>> +				DBG_BUGON(nofail);
>> +				return ERR_PTR(-ENOMEM);
>>  			}
>> -			bio = bio_alloc(gfp, nr_pages);
>> -		} while (bio == NULL);
>> -	}
>> +		}
>> +		bio = bio_alloc(gfp, nr_pages);
> 
> Need to cover this bio_alloc, so should initialize gfp as:
> 
> gfp_t gfp = GFP_NOIO | (nofail ? __GFP_NOFAIL : 0);
> 
> Thanks,
This patch means if bio = bio_alloc(gfp, nr_pages); fails, try to bio_alloc with nr_pages/2, nr_page/4... until 1...
and tag __GFP_NOFAIL with 1...

As you said offline, there is another bug :-(, let me send the next patch...

Thanks,
Gao Xiang

[PREVIEW] [PATCH RESEND 2/4] staging: erofs: introduce erofs_grab_bio

[Chao Yu]

Hi Xiang,

On 2018/8/10 11:01, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/8/10 10:46, Chao Yu wrote:
>> On 2018/8/1 14:04, Gao Xiang wrote:
>>> this patch renames prepare_bio to erofs_grab_bio, and
>>> adds a nofail option in order to retry in the bio allocator.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>> ---
>>>  drivers/staging/erofs/data.c      | 12 ++++++++++--
>>>  drivers/staging/erofs/internal.h  | 35 +++++++++++++++++------------------
>>>  drivers/staging/erofs/unzip_vle.c |  4 ++--
>>>  3 files changed, 29 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>>> index ac263a1..2426eda 100644
>>> --- a/drivers/staging/erofs/data.c
>>> +++ b/drivers/staging/erofs/data.c
>>> @@ -60,7 +60,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
>>>  		struct bio *bio;
>>>  		int err;
>>>  
>>> -		bio = prepare_bio(sb, blkaddr, 1, read_endio);
>>> +		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
>>> +
>>>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
>>>  		BUG_ON(err != PAGE_SIZE);
>>>  
>>> @@ -278,7 +279,14 @@ static inline struct bio *erofs_read_raw_page(
>>>  		if (nblocks > BIO_MAX_PAGES)
>>>  			nblocks = BIO_MAX_PAGES;
>>>  
>>> -		bio = prepare_bio(inode->i_sb, blknr, nblocks, read_endio);
>>> +		bio = erofs_grab_bio(inode->i_sb,
>>> +			blknr, nblocks, read_endio, false);
>>> +
>>> +		if (unlikely(IS_ERR(bio))) {
>>> +			err = PTR_ERR(bio);
>>> +			bio = NULL;
>>> +			goto err_out;
>>> +		}
>>>  	}
>>>  
>>>  	err = bio_add_page(bio, page, PAGE_SIZE, 0);
>>> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
>>> index 4518729..acce2d6 100644
>>> --- a/drivers/staging/erofs/internal.h
>>> +++ b/drivers/staging/erofs/internal.h
>>> @@ -419,26 +419,25 @@ struct erofs_map_blocks {
>>>  #define EROFS_GET_BLOCKS_RAW    0x0001
>>>  
>>>  /* data.c */
>>> -static inline struct bio *prepare_bio(
>>> -	struct super_block *sb,
>>> -	erofs_blk_t blkaddr, unsigned nr_pages,
>>> -	bio_end_io_t endio)
>>> +static inline struct bio *
>>> +erofs_grab_bio(struct super_block *sb,
>>> +	       erofs_blk_t blkaddr, unsigned nr_pages,
>>> +	       bio_end_io_t endio, bool nofail)
>>>  {
>>> -	gfp_t gfp = GFP_NOIO;
>>> -	struct bio *bio = bio_alloc(gfp, nr_pages);
>>> -
>>> -	if (unlikely(bio == NULL) &&
>>> -		(current->flags & PF_MEMALLOC)) {
>>> -		do {
>>> -			nr_pages /= 2;
>>> -			if (unlikely(!nr_pages)) {
>>> -				bio = bio_alloc(gfp | __GFP_NOFAIL, 1);
>>> -				BUG_ON(bio == NULL);
>>> -				break;
>>> +	const gfp_t gfp = GFP_NOIO;
>>> +	struct bio *bio;
>>> +
>>> +	do {
>>> +		if (nr_pages == 1) {
>>> +			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
>>> +			if (unlikely(bio == NULL)) {
>>> +				DBG_BUGON(nofail);
>>> +				return ERR_PTR(-ENOMEM);
>>>  			}
>>> -			bio = bio_alloc(gfp, nr_pages);
>>> -		} while (bio == NULL);
>>> -	}
>>> +		}
>>> +		bio = bio_alloc(gfp, nr_pages);
>>
>> Need to cover this bio_alloc, so should initialize gfp as:
>>
>> gfp_t gfp = GFP_NOIO | (nofail ? __GFP_NOFAIL : 0);
>>
>> Thanks,
> This patch means if bio = bio_alloc(gfp, nr_pages); fails, try to bio_alloc with nr_pages/2, nr_page/4... until 1...
> and tag __GFP_NOFAIL with 1...

Correct, it's okay to me, thanks for explanation. ;)

Thanks,

> 
> As you said offline, there is another bug :-(, let me send the next patch...
> 
> Thanks,
> Gao Xiang
> 
> .
> 

[PREVIEW] [PATCH v3 1/3] staging: erofs: introduce erofs_grab_bio

[Gao Xiang]

this patch renames prepare_bio to erofs_grab_bio, and
adds a nofail option in order to retry in the bio allocator
under memory pressure.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
change log v3:
 - fix a missing break as Chao pointed out online;
 - remove all 'unlikely' hints together with IS_ERR.

 drivers/staging/erofs/data.c      | 12 ++++++++++--
 drivers/staging/erofs/internal.h  | 36 ++++++++++++++++++------------------
 drivers/staging/erofs/unzip_vle.c |  4 ++--
 3 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index ac263a1..e0c046d 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -60,7 +60,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
 		struct bio *bio;
 		int err;
 
-		bio = prepare_bio(sb, blkaddr, 1, read_endio);
+		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
+
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
 		BUG_ON(err != PAGE_SIZE);
 
@@ -278,7 +279,14 @@ static inline struct bio *erofs_read_raw_page(
 		if (nblocks > BIO_MAX_PAGES)
 			nblocks = BIO_MAX_PAGES;
 
-		bio = prepare_bio(inode->i_sb, blknr, nblocks, read_endio);
+		bio = erofs_grab_bio(inode->i_sb,
+			blknr, nblocks, read_endio, false);
+
+		if (IS_ERR(bio)) {
+			err = PTR_ERR(bio);
+			bio = NULL;
+			goto err_out;
+		}
 	}
 
 	err = bio_add_page(bio, page, PAGE_SIZE, 0);
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index b08df0a..79d4e08 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -420,26 +420,26 @@ struct erofs_map_blocks {
 #define EROFS_GET_BLOCKS_RAW    0x0001
 
 /* data.c */
-static inline struct bio *prepare_bio(
-	struct super_block *sb,
-	erofs_blk_t blkaddr, unsigned nr_pages,
-	bio_end_io_t endio)
+static inline struct bio *
+erofs_grab_bio(struct super_block *sb,
+	       erofs_blk_t blkaddr, unsigned int nr_pages,
+	       bio_end_io_t endio, bool nofail)
 {
-	gfp_t gfp = GFP_NOIO;
-	struct bio *bio = bio_alloc(gfp, nr_pages);
-
-	if (unlikely(bio == NULL) &&
-		(current->flags & PF_MEMALLOC)) {
-		do {
-			nr_pages /= 2;
-			if (unlikely(!nr_pages)) {
-				bio = bio_alloc(gfp | __GFP_NOFAIL, 1);
-				BUG_ON(bio == NULL);
-				break;
+	const gfp_t gfp = GFP_NOIO;
+	struct bio *bio;
+
+	do {
+		if (nr_pages == 1) {
+			bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
+			if (unlikely(bio == NULL)) {
+				DBG_BUGON(nofail);
+				return ERR_PTR(-ENOMEM);
 			}
-			bio = bio_alloc(gfp, nr_pages);
-		} while (bio == NULL);
-	}
+			break;
+		}
+		bio = bio_alloc(gfp, nr_pages);
+		nr_pages /= 2;
+	} while (unlikely(bio == NULL));
 
 	bio->bi_end_io = endio;
 	bio_set_dev(bio, sb->s_bdev);
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 45b1255..6bb7aed 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1213,8 +1213,8 @@ static bool z_erofs_vle_submit_all(struct super_block *sb,
 		}
 
 		if (bio == NULL) {
-			bio = prepare_bio(sb, first_index + i,
-				BIO_MAX_PAGES, z_erofs_vle_read_endio);
+			bio = erofs_grab_bio(sb, first_index + i,
+				BIO_MAX_PAGES, z_erofs_vle_read_endio, true);
 			bio->bi_private = tagptr_cast_ptr(bi_private);
 
 			++nr_bios;
-- 
1.9.1

[PREVIEW] [PATCH v3 1/3] staging: erofs: introduce erofs_grab_bio

[Chao Yu]

On 2018/8/11 0:46, Gao Xiang wrote:
> this patch renames prepare_bio to erofs_grab_bio, and
> adds a nofail option in order to retry in the bio allocator
> under memory pressure.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>

Reviewed-by: Chao Yu <yuchao0 at huawei.com>

Thanks,

[PREVIEW] [PATCH 3/4] staging: erofs: seperate erofs_get_meta_page

[Gao Xiang]

This patch seperates 'erofs_get_meta_page' into 'erofs_get_meta_page'
and 'erofs_get_meta_page_nofail'. The second one ensures it should not
fail due to memory pressure.

It also adds const variables in order to fulfill 80 character limit.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/data.c      | 45 +++++++++++++++++++++++----------------
 drivers/staging/erofs/internal.h  | 24 ++++++++++++++++-----
 drivers/staging/erofs/unzip_vle.c | 12 ++++++-----
 drivers/staging/erofs/xattr.c     | 23 ++++++++++++--------
 4 files changed, 67 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index 2426eda..e8637d6 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -39,31 +39,42 @@ static inline void read_endio(struct bio *bio)
 }
 
 /* prio -- true is used for dir */
-struct page *erofs_get_meta_page(struct super_block *sb,
-	erofs_blk_t blkaddr, bool prio)
+struct page *__erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio, bool nofail)
 {
-	struct inode *bd_inode = sb->s_bdev->bd_inode;
-	struct address_space *mapping = bd_inode->i_mapping;
+	struct inode *const bd_inode = sb->s_bdev->bd_inode;
+	struct address_space *const mapping = bd_inode->i_mapping;
+	/* prefer retrying in the allocator to blindly looping below */
+	const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
+		(nofail ? __GFP_NOFAIL : 0);
 	struct page *page;
 
 repeat:
-	page = find_or_create_page(mapping, blkaddr,
-	/*
-	 * Prefer looping in the allocator rather than here,
-	 * at least that code knows what it's doing.
-	 */
-		mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL);
-
-	BUG_ON(!page || !PageLocked(page));
+	page = find_or_create_page(mapping, blkaddr, gfp);
+	if (unlikely(page == NULL)) {
+		DBG_BUGON(nofail);
+		return ERR_PTR(-ENOMEM);
+	}
 
 	if (!PageUptodate(page)) {
 		struct bio *bio;
 		int err;
 
-		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
+		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, nofail);
+		if (unlikely(bio == NULL)) {
+			DBG_BUGON(nofail);
+			err = -ENOMEM;
+err_out:
+			unlock_page(page);
+			put_page(page);
+			return ERR_PTR(err);
+		}
 
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		BUG_ON(err != PAGE_SIZE);
+		if (unlikely(err != PAGE_SIZE)) {
+			err = -EFAULT;
+			goto err_out;
+		}
 
 		__submit_bio(bio, REQ_OP_READ,
 			REQ_META | (prio ? REQ_PRIO : 0));
@@ -79,10 +90,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
 
 		/* more likely a read error */
 		if (unlikely(!PageUptodate(page))) {
-			unlock_page(page);
-			put_page(page);
-
-			page = ERR_PTR(-EIO);
+			err = -EIO;
+			goto err_out;
 		}
 	}
 	return page;
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index acce2d6..cc47b0c 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -451,8 +451,21 @@ static inline void __submit_bio(struct bio *bio, unsigned op, unsigned op_flags)
 	submit_bio(bio);
 }
 
-extern struct page *erofs_get_meta_page(struct super_block *sb,
-	erofs_blk_t blkaddr, bool prio);
+extern struct page *__erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio, bool nofail);
+
+static inline struct page *erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio)
+{
+	return __erofs_get_meta_page(sb, blkaddr, prio, false);
+}
+
+static inline struct page *erofs_get_meta_page_nofail(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio)
+{
+	return __erofs_get_meta_page(sb, blkaddr, prio, true);
+}
+
 extern int erofs_map_blocks(struct inode *, struct erofs_map_blocks *, int);
 extern int erofs_map_blocks_iter(struct inode *, struct erofs_map_blocks *,
 	struct page **, int);
@@ -463,10 +476,11 @@ struct erofs_map_blocks_iter {
 };
 
 
-static inline struct page *erofs_get_inline_page(struct inode *inode,
-	erofs_blk_t blkaddr)
+static inline struct page *
+erofs_get_inline_page_nofail(struct inode *inode,
+			     erofs_blk_t blkaddr)
 {
-	return erofs_get_meta_page(inode->i_sb,
+	return erofs_get_meta_page_nofail(inode->i_sb,
 		blkaddr, S_ISDIR(inode->i_mode));
 }
 
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index be3929d..823ea2e 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1487,7 +1487,8 @@ static erofs_off_t vle_get_logical_extent_head(
 	erofs_blk_t blkaddr = vle_extent_blkaddr(inode, lcn);
 	struct z_erofs_vle_decompressed_index *di;
 	unsigned long long ofs;
-	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
+	struct super_block *const sb = inode->i_sb;
+	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
 	const unsigned int clustersize = 1 << clusterbits;
 
 	if (page->index != blkaddr) {
@@ -1495,8 +1496,8 @@ static erofs_off_t vle_get_logical_extent_head(
 		unlock_page(page);
 		put_page(page);
 
-		*page_iter = page = erofs_get_meta_page(inode->i_sb,
-			blkaddr, false);
+		page = erofs_get_meta_page_nofail(sb, blkaddr, false);
+		*page_iter = page;
 		*kaddr_iter = kmap_atomic(page);
 	}
 
@@ -1537,7 +1538,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	struct page *mpage = *mpage_ret;
 	void *kaddr;
 	bool initial;
-	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
+	struct super_block *const sb = inode->i_sb;
+	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
 	const unsigned int clustersize = 1 << clusterbits;
 	int err = 0;
 
@@ -1568,7 +1570,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 		if (mpage != NULL)
 			put_page(mpage);
 
-		mpage = erofs_get_meta_page(inode->i_sb, e_blkaddr, false);
+		mpage = erofs_get_meta_page_nofail(sb, e_blkaddr, false);
 		*mpage_ret = mpage;
 	} else {
 		lock_page(mpage);
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 6b9685f..702434b 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -38,6 +38,7 @@ static void init_inode_xattrs(struct inode *inode)
 	struct xattr_iter it;
 	unsigned i;
 	struct erofs_xattr_ibody_header *ih;
+	struct super_block *sb;
 	struct erofs_sb_info *sbi;
 	struct erofs_vnode *vi;
 	bool atomic_map;
@@ -48,11 +49,12 @@ static void init_inode_xattrs(struct inode *inode)
 	vi = EROFS_V(inode);
 	BUG_ON(!vi->xattr_isize);
 
-	sbi = EROFS_I_SB(inode);
+	sb = inode->i_sb;
+	sbi = EROFS_SB(sb);
 	it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
 	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
 
-	it.page = erofs_get_inline_page(inode, it.blkaddr);
+	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
 	BUG_ON(IS_ERR(it.page));
 
 	/* read in shared xattr array (non-atomic, see kmalloc below) */
@@ -75,7 +77,7 @@ static void init_inode_xattrs(struct inode *inode)
 			BUG_ON(it.ofs != EROFS_BLKSIZ);
 			xattr_iter_end(&it, atomic_map);
 
-			it.page = erofs_get_meta_page(inode->i_sb,
+			it.page = erofs_get_meta_page_nofail(sb,
 				++it.blkaddr, S_ISDIR(inode->i_mode));
 			BUG_ON(IS_ERR(it.page));
 
@@ -105,7 +107,8 @@ static void xattr_iter_fixup(struct xattr_iter *it)
 		xattr_iter_end(it, true);
 
 		it->blkaddr += erofs_blknr(it->ofs);
-		it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
+		it->page = erofs_get_meta_page_nofail(it->sb,
+			it->blkaddr, false);
 		BUG_ON(IS_ERR(it->page));
 
 		it->kaddr = kmap_atomic(it->page);
@@ -131,7 +134,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
 	it->blkaddr = erofs_blknr(iloc(sbi, vi->nid) + inline_xattr_ofs);
 	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
 
-	it->page = erofs_get_inline_page(inode, it->blkaddr);
+	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
 	BUG_ON(IS_ERR(it->page));
 	it->kaddr = kmap_atomic(it->page);
 
@@ -300,7 +303,8 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
 static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 {
 	struct erofs_vnode *const vi = EROFS_V(inode);
-	struct erofs_sb_info *const sbi = EROFS_SB(inode->i_sb);
+	struct super_block *const sb = inode->i_sb;
+	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 	unsigned i;
 	int ret = -ENOATTR;
 
@@ -314,7 +318,7 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page(inode->i_sb,
+			it->it.page = erofs_get_meta_page_nofail(sb,
 				blkaddr, false);
 			BUG_ON(IS_ERR(it->it.page));
 			it->it.kaddr = kmap_atomic(it->it.page);
@@ -524,7 +528,8 @@ static int shared_listxattr(struct listxattr_iter *it)
 {
 	struct inode *const inode = d_inode(it->dentry);
 	struct erofs_vnode *const vi = EROFS_V(inode);
-	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
+	struct super_block *const sb = inode->i_sb;
+	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 	unsigned i;
 	int ret = 0;
 
@@ -537,7 +542,7 @@ static int shared_listxattr(struct listxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page(inode->i_sb,
+			it->it.page = erofs_get_meta_page_nofail(sb,
 				blkaddr, false);
 			BUG_ON(IS_ERR(it->it.page));
 			it->it.kaddr = kmap_atomic(it->it.page);
-- 
1.9.1

[PREVIEW] [PATCH 3/4] staging: erofs: seperate erofs_get_meta_page

[Chao Yu]

On 2018/8/1 14:01, Gao Xiang wrote:
> This patch seperates 'erofs_get_meta_page' into 'erofs_get_meta_page'
> and 'erofs_get_meta_page_nofail'. The second one ensures it should not
> fail due to memory pressure.
> 
> It also adds const variables in order to fulfill 80 character limit.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
> ---
>  drivers/staging/erofs/data.c      | 45 +++++++++++++++++++++++----------------
>  drivers/staging/erofs/internal.h  | 24 ++++++++++++++++-----
>  drivers/staging/erofs/unzip_vle.c | 12 ++++++-----
>  drivers/staging/erofs/xattr.c     | 23 ++++++++++++--------
>  4 files changed, 67 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index 2426eda..e8637d6 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -39,31 +39,42 @@ static inline void read_endio(struct bio *bio)
>  }
>  
>  /* prio -- true is used for dir */
> -struct page *erofs_get_meta_page(struct super_block *sb,
> -	erofs_blk_t blkaddr, bool prio)
> +struct page *__erofs_get_meta_page(struct super_block *sb,
> +	erofs_blk_t blkaddr, bool prio, bool nofail)
>  {
> -	struct inode *bd_inode = sb->s_bdev->bd_inode;
> -	struct address_space *mapping = bd_inode->i_mapping;
> +	struct inode *const bd_inode = sb->s_bdev->bd_inode;
> +	struct address_space *const mapping = bd_inode->i_mapping;
> +	/* prefer retrying in the allocator to blindly looping below */
> +	const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
> +		(nofail ? __GFP_NOFAIL : 0);
>  	struct page *page;
>  
>  repeat:
> -	page = find_or_create_page(mapping, blkaddr,
> -	/*
> -	 * Prefer looping in the allocator rather than here,
> -	 * at least that code knows what it's doing.
> -	 */
> -		mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL);
> -
> -	BUG_ON(!page || !PageLocked(page));
> +	page = find_or_create_page(mapping, blkaddr, gfp);
> +	if (unlikely(page == NULL)) {
> +		DBG_BUGON(nofail);
> +		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	if (!PageUptodate(page)) {
>  		struct bio *bio;
>  		int err;
>  
> -		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
> +		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, nofail);
> +		if (unlikely(bio == NULL)) {
> +			DBG_BUGON(nofail);
> +			err = -ENOMEM;
> +err_out:
> +			unlock_page(page);
> +			put_page(page);
> +			return ERR_PTR(err);
> +		}
>  
>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
> -		BUG_ON(err != PAGE_SIZE);
> +		if (unlikely(err != PAGE_SIZE)) {
> +			err = -EFAULT;
> +			goto err_out;
> +		}
>  
>  		__submit_bio(bio, REQ_OP_READ,
>  			REQ_META | (prio ? REQ_PRIO : 0));
> @@ -79,10 +90,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
>  
>  		/* more likely a read error */
>  		if (unlikely(!PageUptodate(page))) {
> -			unlock_page(page);
> -			put_page(page);
> -
> -			page = ERR_PTR(-EIO);

DBG_BUGON(nofail);

Thanks,

> +			err = -EIO;
> +			goto err_out;
>  		}
>  	}
>  	return page;
> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
> index acce2d6..cc47b0c 100644
> --- a/drivers/staging/erofs/internal.h
> +++ b/drivers/staging/erofs/internal.h
> @@ -451,8 +451,21 @@ static inline void __submit_bio(struct bio *bio, unsigned op, unsigned op_flags)
>  	submit_bio(bio);
>  }
>  
> -extern struct page *erofs_get_meta_page(struct super_block *sb,
> -	erofs_blk_t blkaddr, bool prio);
> +extern struct page *__erofs_get_meta_page(struct super_block *sb,
> +	erofs_blk_t blkaddr, bool prio, bool nofail);
> +
> +static inline struct page *erofs_get_meta_page(struct super_block *sb,
> +	erofs_blk_t blkaddr, bool prio)
> +{
> +	return __erofs_get_meta_page(sb, blkaddr, prio, false);
> +}
> +
> +static inline struct page *erofs_get_meta_page_nofail(struct super_block *sb,
> +	erofs_blk_t blkaddr, bool prio)
> +{
> +	return __erofs_get_meta_page(sb, blkaddr, prio, true);
> +}
> +
>  extern int erofs_map_blocks(struct inode *, struct erofs_map_blocks *, int);
>  extern int erofs_map_blocks_iter(struct inode *, struct erofs_map_blocks *,
>  	struct page **, int);
> @@ -463,10 +476,11 @@ struct erofs_map_blocks_iter {
>  };
>  
>  
> -static inline struct page *erofs_get_inline_page(struct inode *inode,
> -	erofs_blk_t blkaddr)
> +static inline struct page *
> +erofs_get_inline_page_nofail(struct inode *inode,
> +			     erofs_blk_t blkaddr)
>  {
> -	return erofs_get_meta_page(inode->i_sb,
> +	return erofs_get_meta_page_nofail(inode->i_sb,
>  		blkaddr, S_ISDIR(inode->i_mode));
>  }
>  
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index be3929d..823ea2e 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -1487,7 +1487,8 @@ static erofs_off_t vle_get_logical_extent_head(
>  	erofs_blk_t blkaddr = vle_extent_blkaddr(inode, lcn);
>  	struct z_erofs_vle_decompressed_index *di;
>  	unsigned long long ofs;
> -	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
> +	struct super_block *const sb = inode->i_sb;
> +	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
>  	const unsigned int clustersize = 1 << clusterbits;
>  
>  	if (page->index != blkaddr) {
> @@ -1495,8 +1496,8 @@ static erofs_off_t vle_get_logical_extent_head(
>  		unlock_page(page);
>  		put_page(page);
>  
> -		*page_iter = page = erofs_get_meta_page(inode->i_sb,
> -			blkaddr, false);
> +		page = erofs_get_meta_page_nofail(sb, blkaddr, false);
> +		*page_iter = page;
>  		*kaddr_iter = kmap_atomic(page);
>  	}
>  
> @@ -1537,7 +1538,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>  	struct page *mpage = *mpage_ret;
>  	void *kaddr;
>  	bool initial;
> -	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
> +	struct super_block *const sb = inode->i_sb;
> +	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
>  	const unsigned int clustersize = 1 << clusterbits;
>  	int err = 0;
>  
> @@ -1568,7 +1570,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>  		if (mpage != NULL)
>  			put_page(mpage);
>  
> -		mpage = erofs_get_meta_page(inode->i_sb, e_blkaddr, false);
> +		mpage = erofs_get_meta_page_nofail(sb, e_blkaddr, false);
>  		*mpage_ret = mpage;
>  	} else {
>  		lock_page(mpage);
> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
> index 6b9685f..702434b 100644
> --- a/drivers/staging/erofs/xattr.c
> +++ b/drivers/staging/erofs/xattr.c
> @@ -38,6 +38,7 @@ static void init_inode_xattrs(struct inode *inode)
>  	struct xattr_iter it;
>  	unsigned i;
>  	struct erofs_xattr_ibody_header *ih;
> +	struct super_block *sb;
>  	struct erofs_sb_info *sbi;
>  	struct erofs_vnode *vi;
>  	bool atomic_map;
> @@ -48,11 +49,12 @@ static void init_inode_xattrs(struct inode *inode)
>  	vi = EROFS_V(inode);
>  	BUG_ON(!vi->xattr_isize);
>  
> -	sbi = EROFS_I_SB(inode);
> +	sb = inode->i_sb;
> +	sbi = EROFS_SB(sb);
>  	it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
>  	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>  
> -	it.page = erofs_get_inline_page(inode, it.blkaddr);
> +	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
>  	BUG_ON(IS_ERR(it.page));
>  
>  	/* read in shared xattr array (non-atomic, see kmalloc below) */
> @@ -75,7 +77,7 @@ static void init_inode_xattrs(struct inode *inode)
>  			BUG_ON(it.ofs != EROFS_BLKSIZ);
>  			xattr_iter_end(&it, atomic_map);
>  
> -			it.page = erofs_get_meta_page(inode->i_sb,
> +			it.page = erofs_get_meta_page_nofail(sb,
>  				++it.blkaddr, S_ISDIR(inode->i_mode));
>  			BUG_ON(IS_ERR(it.page));
>  
> @@ -105,7 +107,8 @@ static void xattr_iter_fixup(struct xattr_iter *it)
>  		xattr_iter_end(it, true);
>  
>  		it->blkaddr += erofs_blknr(it->ofs);
> -		it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
> +		it->page = erofs_get_meta_page_nofail(it->sb,
> +			it->blkaddr, false);
>  		BUG_ON(IS_ERR(it->page));
>  
>  		it->kaddr = kmap_atomic(it->page);
> @@ -131,7 +134,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
>  	it->blkaddr = erofs_blknr(iloc(sbi, vi->nid) + inline_xattr_ofs);
>  	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
>  
> -	it->page = erofs_get_inline_page(inode, it->blkaddr);
> +	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
>  	BUG_ON(IS_ERR(it->page));
>  	it->kaddr = kmap_atomic(it->page);
>  
> @@ -300,7 +303,8 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
>  static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>  {
>  	struct erofs_vnode *const vi = EROFS_V(inode);
> -	struct erofs_sb_info *const sbi = EROFS_SB(inode->i_sb);
> +	struct super_block *const sb = inode->i_sb;
> +	struct erofs_sb_info *const sbi = EROFS_SB(sb);
>  	unsigned i;
>  	int ret = -ENOATTR;
>  
> @@ -314,7 +318,7 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>  			if (i)
>  				xattr_iter_end(&it->it, true);
>  
> -			it->it.page = erofs_get_meta_page(inode->i_sb,
> +			it->it.page = erofs_get_meta_page_nofail(sb,
>  				blkaddr, false);
>  			BUG_ON(IS_ERR(it->it.page));
>  			it->it.kaddr = kmap_atomic(it->it.page);
> @@ -524,7 +528,8 @@ static int shared_listxattr(struct listxattr_iter *it)
>  {
>  	struct inode *const inode = d_inode(it->dentry);
>  	struct erofs_vnode *const vi = EROFS_V(inode);
> -	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
> +	struct super_block *const sb = inode->i_sb;
> +	struct erofs_sb_info *const sbi = EROFS_SB(sb);
>  	unsigned i;
>  	int ret = 0;
>  
> @@ -537,7 +542,7 @@ static int shared_listxattr(struct listxattr_iter *it)
>  			if (i)
>  				xattr_iter_end(&it->it, true);
>  
> -			it->it.page = erofs_get_meta_page(inode->i_sb,
> +			it->it.page = erofs_get_meta_page_nofail(sb,
>  				blkaddr, false);
>  			BUG_ON(IS_ERR(it->it.page));
>  			it->it.kaddr = kmap_atomic(it->it.page);
> 

[PREVIEW] [PATCH 3/4] staging: erofs: seperate erofs_get_meta_page

[Gao Xiang]

Hi Chao,

On 2018/8/10 11:05, Chao Yu wrote:
> On 2018/8/1 14:01, Gao Xiang wrote:
>> This patch seperates 'erofs_get_meta_page' into 'erofs_get_meta_page'
>> and 'erofs_get_meta_page_nofail'. The second one ensures it should not
>> fail due to memory pressure.
>>
>> It also adds const variables in order to fulfill 80 character limit.
>>
>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>> ---
>>  drivers/staging/erofs/data.c      | 45 +++++++++++++++++++++++----------------
>>  drivers/staging/erofs/internal.h  | 24 ++++++++++++++++-----
>>  drivers/staging/erofs/unzip_vle.c | 12 ++++++-----
>>  drivers/staging/erofs/xattr.c     | 23 ++++++++++++--------
>>  4 files changed, 67 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>> index 2426eda..e8637d6 100644
>> --- a/drivers/staging/erofs/data.c
>> +++ b/drivers/staging/erofs/data.c
>> @@ -39,31 +39,42 @@ static inline void read_endio(struct bio *bio)
>>  }
>>  
>>  /* prio -- true is used for dir */
>> -struct page *erofs_get_meta_page(struct super_block *sb,
>> -	erofs_blk_t blkaddr, bool prio)
>> +struct page *__erofs_get_meta_page(struct super_block *sb,
>> +	erofs_blk_t blkaddr, bool prio, bool nofail)
>>  {
>> -	struct inode *bd_inode = sb->s_bdev->bd_inode;
>> -	struct address_space *mapping = bd_inode->i_mapping;
>> +	struct inode *const bd_inode = sb->s_bdev->bd_inode;
>> +	struct address_space *const mapping = bd_inode->i_mapping;
>> +	/* prefer retrying in the allocator to blindly looping below */
>> +	const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
>> +		(nofail ? __GFP_NOFAIL : 0);
>>  	struct page *page;
>>  
>>  repeat:
>> -	page = find_or_create_page(mapping, blkaddr,
>> -	/*
>> -	 * Prefer looping in the allocator rather than here,
>> -	 * at least that code knows what it's doing.
>> -	 */
>> -		mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL);
>> -
>> -	BUG_ON(!page || !PageLocked(page));
>> +	page = find_or_create_page(mapping, blkaddr, gfp);
>> +	if (unlikely(page == NULL)) {
>> +		DBG_BUGON(nofail);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>>  
>>  	if (!PageUptodate(page)) {
>>  		struct bio *bio;
>>  		int err;
>>  
>> -		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
>> +		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, nofail);
>> +		if (unlikely(bio == NULL)) {
>> +			DBG_BUGON(nofail);
>> +			err = -ENOMEM;
>> +err_out:
>> +			unlock_page(page);
>> +			put_page(page);
>> +			return ERR_PTR(err);
>> +		}
>>  
>>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
>> -		BUG_ON(err != PAGE_SIZE);
>> +		if (unlikely(err != PAGE_SIZE)) {
>> +			err = -EFAULT;
>> +			goto err_out;
>> +		}
>>  
>>  		__submit_bio(bio, REQ_OP_READ,
>>  			REQ_META | (prio ? REQ_PRIO : 0));
>> @@ -79,10 +90,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
>>  
>>  		/* more likely a read error */
>>  		if (unlikely(!PageUptodate(page))) {
>> -			unlock_page(page);
>> -			put_page(page);
>> -
>> -			page = ERR_PTR(-EIO);
> DBG_BUGON(nofail);
> 
> Thanks,
> 

nofail in erofs means "no fail under memory pressure", if !PageUptodate(page) after endio,
it means "a IO read error occurred", so I think it could happen on disk error and
it is not suitable to guarantee disk error nofail (...may be cause EIO again and again...).......

Thanks,
Gao Xiang

[PREVIEW] [PATCH 3/4] staging: erofs: seperate erofs_get_meta_page

[Chao Yu]

Hi Xiang,

On 2018/8/10 11:28, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/8/10 11:05, Chao Yu wrote:
>> On 2018/8/1 14:01, Gao Xiang wrote:
>>> This patch seperates 'erofs_get_meta_page' into 'erofs_get_meta_page'
>>> and 'erofs_get_meta_page_nofail'. The second one ensures it should not
>>> fail due to memory pressure.
>>>
>>> It also adds const variables in order to fulfill 80 character limit.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>> ---
>>>  drivers/staging/erofs/data.c      | 45 +++++++++++++++++++++++----------------
>>>  drivers/staging/erofs/internal.h  | 24 ++++++++++++++++-----
>>>  drivers/staging/erofs/unzip_vle.c | 12 ++++++-----
>>>  drivers/staging/erofs/xattr.c     | 23 ++++++++++++--------
>>>  4 files changed, 67 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>>> index 2426eda..e8637d6 100644
>>> --- a/drivers/staging/erofs/data.c
>>> +++ b/drivers/staging/erofs/data.c
>>> @@ -39,31 +39,42 @@ static inline void read_endio(struct bio *bio)
>>>  }
>>>  
>>>  /* prio -- true is used for dir */
>>> -struct page *erofs_get_meta_page(struct super_block *sb,
>>> -	erofs_blk_t blkaddr, bool prio)
>>> +struct page *__erofs_get_meta_page(struct super_block *sb,
>>> +	erofs_blk_t blkaddr, bool prio, bool nofail)
>>>  {
>>> -	struct inode *bd_inode = sb->s_bdev->bd_inode;
>>> -	struct address_space *mapping = bd_inode->i_mapping;
>>> +	struct inode *const bd_inode = sb->s_bdev->bd_inode;
>>> +	struct address_space *const mapping = bd_inode->i_mapping;
>>> +	/* prefer retrying in the allocator to blindly looping below */
>>> +	const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
>>> +		(nofail ? __GFP_NOFAIL : 0);
>>>  	struct page *page;
>>>  
>>>  repeat:
>>> -	page = find_or_create_page(mapping, blkaddr,
>>> -	/*
>>> -	 * Prefer looping in the allocator rather than here,
>>> -	 * at least that code knows what it's doing.
>>> -	 */
>>> -		mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL);
>>> -
>>> -	BUG_ON(!page || !PageLocked(page));
>>> +	page = find_or_create_page(mapping, blkaddr, gfp);
>>> +	if (unlikely(page == NULL)) {
>>> +		DBG_BUGON(nofail);
>>> +		return ERR_PTR(-ENOMEM);
>>> +	}
>>>  
>>>  	if (!PageUptodate(page)) {
>>>  		struct bio *bio;
>>>  		int err;
>>>  
>>> -		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
>>> +		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, nofail);
>>> +		if (unlikely(bio == NULL)) {
>>> +			DBG_BUGON(nofail);
>>> +			err = -ENOMEM;
>>> +err_out:
>>> +			unlock_page(page);
>>> +			put_page(page);
>>> +			return ERR_PTR(err);
>>> +		}
>>>  
>>>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
>>> -		BUG_ON(err != PAGE_SIZE);
>>> +		if (unlikely(err != PAGE_SIZE)) {
>>> +			err = -EFAULT;
>>> +			goto err_out;
>>> +		}
>>>  
>>>  		__submit_bio(bio, REQ_OP_READ,
>>>  			REQ_META | (prio ? REQ_PRIO : 0));
>>> @@ -79,10 +90,8 @@ struct page *erofs_get_meta_page(struct super_block *sb,
>>>  
>>>  		/* more likely a read error */
>>>  		if (unlikely(!PageUptodate(page))) {
>>> -			unlock_page(page);
>>> -			put_page(page);
>>> -
>>> -			page = ERR_PTR(-EIO);
>> DBG_BUGON(nofail);
>>
>> Thanks,
>>
> 
> nofail in erofs means "no fail under memory pressure", if !PageUptodate(page) after endio,

Yup, that matches your commit log.

> it means "a IO read error occurred", so I think it could happen on disk error and
> it is not suitable to guarantee disk error nofail (...may be cause EIO again and again...).......

IMO, the purpose of introducing 'nofail' function is to make caller simplifying
its error handling, since callee can handle any error insidely, if callee still
can fail due to other reason like -EIO, we still need another handling in caller.

So here, if we encounter -EIO, how about retrying several time in
erofs_get_meta_page_nofail like f2fs did now?

Thanks,

> 
> Thanks,
> Gao Xiang
> 
> 
> .
> 

[PREVIEW] [PATCH 3/4] staging: erofs: seperate erofs_get_meta_page

[Gao Xiang]

Hi Chao,

On 2018/8/10 14:02, Chao Yu wrote:
> IMO, the purpose of introducing 'nofail' function is to make caller simplifying
> its error handling, since callee can handle any error insidely, if callee still
> can fail due to other reason like -EIO, we still need another handling in caller.
> 
> So here, if we encounter -EIO, how about retrying several time in
> erofs_get_meta_page_nofail like f2fs did now?
> 

So it can be, let me introducing another io retry count in Kconfig to handle that,
will see in the next patch :)

Thanks,
Gao Xiang

> Thanks,

[PREVIEW] [PATCH v3 2/3] staging: erofs: separate erofs_get_meta_page

[Gao Xiang]

This patch separates 'erofs_get_meta_page' into 'erofs_get_meta_page'
and 'erofs_get_meta_page_nofail'. The second one ensures that it
should not fail under memory pressure and should make best efforts
if IO errors occur.

It also adds const variables in order to fulfill 80 character limit.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
change log v3:
 - add EROFS_FS_IO_MAX_RETRIES to handle IO Error for nofail cases
   according to Chao's suggestion;
 - remove all 'unlikely' hints together with IS_ERR.

 drivers/staging/erofs/Kconfig     |  9 +++++++
 drivers/staging/erofs/data.c      | 52 +++++++++++++++++++++++++--------------
 drivers/staging/erofs/internal.h  | 24 ++++++++++++++----
 drivers/staging/erofs/unzip_vle.c | 12 +++++----
 drivers/staging/erofs/xattr.c     | 23 ++++++++++-------
 5 files changed, 83 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/erofs/Kconfig b/drivers/staging/erofs/Kconfig
index 96f6149..7f209b3 100644
--- a/drivers/staging/erofs/Kconfig
+++ b/drivers/staging/erofs/Kconfig
@@ -78,6 +78,15 @@ config EROFS_FAULT_INJECTION
 	  Test EROFS to inject faults such as ENOMEM, EIO, and so on.
 	  If unsure, say N.
 
+config EROFS_FS_IO_MAX_RETRIES
+	int "EROFS IO Maximum Retries"
+	depends on EROFS_FS
+	default "5"
+	help
+	  Maximum retry count of IO Errors.
+
+	  If unsure, leave the default value (5 retries, 6 IOs at most).
+
 config EROFS_FS_ZIP
 	bool "EROFS Data Compresssion Support"
 	depends on EROFS_FS
diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index e0c046d..908b774 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -39,31 +39,44 @@ static inline void read_endio(struct bio *bio)
 }
 
 /* prio -- true is used for dir */
-struct page *erofs_get_meta_page(struct super_block *sb,
-	erofs_blk_t blkaddr, bool prio)
+struct page *__erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio, bool nofail)
 {
-	struct inode *bd_inode = sb->s_bdev->bd_inode;
-	struct address_space *mapping = bd_inode->i_mapping;
+	struct inode *const bd_inode = sb->s_bdev->bd_inode;
+	struct address_space *const mapping = bd_inode->i_mapping;
+	/* prefer retrying in the allocator to blindly looping below */
+	const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
+		(nofail ? __GFP_NOFAIL : 0);
+	unsigned int io_retries = nofail ? CONFIG_EROFS_FS_IO_MAX_RETRIES : 0;
 	struct page *page;
 
 repeat:
-	page = find_or_create_page(mapping, blkaddr,
-	/*
-	 * Prefer looping in the allocator rather than here,
-	 * at least that code knows what it's doing.
-	 */
-		mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL);
-
-	BUG_ON(!page || !PageLocked(page));
+	page = find_or_create_page(mapping, blkaddr, gfp);
+	if (unlikely(page == NULL)) {
+		DBG_BUGON(nofail);
+		return ERR_PTR(-ENOMEM);
+	}
+	DBG_BUGON(!PageLocked(page));
 
 	if (!PageUptodate(page)) {
 		struct bio *bio;
 		int err;
 
-		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
+		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, nofail);
+		if (unlikely(bio == NULL)) {
+			DBG_BUGON(nofail);
+			err = -ENOMEM;
+err_out:
+			unlock_page(page);
+			put_page(page);
+			return ERR_PTR(err);
+		}
 
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		BUG_ON(err != PAGE_SIZE);
+		if (unlikely(err != PAGE_SIZE)) {
+			err = -EFAULT;
+			goto err_out;
+		}
 
 		__submit_bio(bio, REQ_OP_READ,
 			REQ_META | (prio ? REQ_PRIO : 0));
@@ -72,6 +85,7 @@ struct page *erofs_get_meta_page(struct super_block *sb,
 
 		/* the page has been truncated by others? */
 		if (unlikely(page->mapping != mapping)) {
+unlock_repeat:
 			unlock_page(page);
 			put_page(page);
 			goto repeat;
@@ -79,10 +93,12 @@ struct page *erofs_get_meta_page(struct super_block *sb,
 
 		/* more likely a read error */
 		if (unlikely(!PageUptodate(page))) {
-			unlock_page(page);
-			put_page(page);
-
-			page = ERR_PTR(-EIO);
+			if (io_retries) {
+				--io_retries;
+				goto unlock_repeat;
+			}
+			err = -EIO;
+			goto err_out;
 		}
 	}
 	return page;
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 79d4e08..ecb6c02 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -453,8 +453,21 @@ static inline void __submit_bio(struct bio *bio, unsigned op, unsigned op_flags)
 	submit_bio(bio);
 }
 
-extern struct page *erofs_get_meta_page(struct super_block *sb,
-	erofs_blk_t blkaddr, bool prio);
+extern struct page *__erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio, bool nofail);
+
+static inline struct page *erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio)
+{
+	return __erofs_get_meta_page(sb, blkaddr, prio, false);
+}
+
+static inline struct page *erofs_get_meta_page_nofail(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio)
+{
+	return __erofs_get_meta_page(sb, blkaddr, prio, true);
+}
+
 extern int erofs_map_blocks(struct inode *, struct erofs_map_blocks *, int);
 extern int erofs_map_blocks_iter(struct inode *, struct erofs_map_blocks *,
 	struct page **, int);
@@ -465,10 +478,11 @@ struct erofs_map_blocks_iter {
 };
 
 
-static inline struct page *erofs_get_inline_page(struct inode *inode,
-	erofs_blk_t blkaddr)
+static inline struct page *
+erofs_get_inline_page_nofail(struct inode *inode,
+			     erofs_blk_t blkaddr)
 {
-	return erofs_get_meta_page(inode->i_sb,
+	return erofs_get_meta_page_nofail(inode->i_sb,
 		blkaddr, S_ISDIR(inode->i_mode));
 }
 
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 6bb7aed..d7692a2 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1488,7 +1488,8 @@ static erofs_off_t vle_get_logical_extent_head(
 	erofs_blk_t blkaddr = vle_extent_blkaddr(inode, lcn);
 	struct z_erofs_vle_decompressed_index *di;
 	unsigned long long ofs;
-	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
+	struct super_block *const sb = inode->i_sb;
+	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
 	const unsigned int clustersize = 1 << clusterbits;
 
 	if (page->index != blkaddr) {
@@ -1496,8 +1497,8 @@ static erofs_off_t vle_get_logical_extent_head(
 		unlock_page(page);
 		put_page(page);
 
-		*page_iter = page = erofs_get_meta_page(inode->i_sb,
-			blkaddr, false);
+		page = erofs_get_meta_page_nofail(sb, blkaddr, false);
+		*page_iter = page;
 		*kaddr_iter = kmap_atomic(page);
 	}
 
@@ -1538,7 +1539,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	struct page *mpage = *mpage_ret;
 	void *kaddr;
 	bool initial;
-	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
+	struct super_block *const sb = inode->i_sb;
+	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
 	const unsigned int clustersize = 1 << clusterbits;
 	int err = 0;
 
@@ -1569,7 +1571,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 		if (mpage != NULL)
 			put_page(mpage);
 
-		mpage = erofs_get_meta_page(inode->i_sb, e_blkaddr, false);
+		mpage = erofs_get_meta_page_nofail(sb, e_blkaddr, false);
 		*mpage_ret = mpage;
 	} else {
 		lock_page(mpage);
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 0e9cfec..2593c85 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -38,6 +38,7 @@ static void init_inode_xattrs(struct inode *inode)
 	struct xattr_iter it;
 	unsigned i;
 	struct erofs_xattr_ibody_header *ih;
+	struct super_block *sb;
 	struct erofs_sb_info *sbi;
 	struct erofs_vnode *vi;
 	bool atomic_map;
@@ -48,11 +49,12 @@ static void init_inode_xattrs(struct inode *inode)
 	vi = EROFS_V(inode);
 	BUG_ON(!vi->xattr_isize);
 
-	sbi = EROFS_I_SB(inode);
+	sb = inode->i_sb;
+	sbi = EROFS_SB(sb);
 	it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
 	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
 
-	it.page = erofs_get_inline_page(inode, it.blkaddr);
+	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
 	BUG_ON(IS_ERR(it.page));
 
 	/* read in shared xattr array (non-atomic, see kmalloc below) */
@@ -75,7 +77,7 @@ static void init_inode_xattrs(struct inode *inode)
 			BUG_ON(it.ofs != EROFS_BLKSIZ);
 			xattr_iter_end(&it, atomic_map);
 
-			it.page = erofs_get_meta_page(inode->i_sb,
+			it.page = erofs_get_meta_page_nofail(sb,
 				++it.blkaddr, S_ISDIR(inode->i_mode));
 			BUG_ON(IS_ERR(it.page));
 
@@ -105,7 +107,8 @@ static void xattr_iter_fixup(struct xattr_iter *it)
 		xattr_iter_end(it, true);
 
 		it->blkaddr += erofs_blknr(it->ofs);
-		it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
+		it->page = erofs_get_meta_page_nofail(it->sb,
+			it->blkaddr, false);
 		BUG_ON(IS_ERR(it->page));
 
 		it->kaddr = kmap_atomic(it->page);
@@ -131,7 +134,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
 	it->blkaddr = erofs_blknr(iloc(sbi, vi->nid) + inline_xattr_ofs);
 	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
 
-	it->page = erofs_get_inline_page(inode, it->blkaddr);
+	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
 	BUG_ON(IS_ERR(it->page));
 	it->kaddr = kmap_atomic(it->page);
 
@@ -300,7 +303,8 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
 static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 {
 	struct erofs_vnode *const vi = EROFS_V(inode);
-	struct erofs_sb_info *const sbi = EROFS_SB(inode->i_sb);
+	struct super_block *const sb = inode->i_sb;
+	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 	unsigned i;
 	int ret = -ENOATTR;
 
@@ -314,7 +318,7 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page(inode->i_sb,
+			it->it.page = erofs_get_meta_page_nofail(sb,
 				blkaddr, false);
 			BUG_ON(IS_ERR(it->it.page));
 			it->it.kaddr = kmap_atomic(it->it.page);
@@ -524,7 +528,8 @@ static int shared_listxattr(struct listxattr_iter *it)
 {
 	struct inode *const inode = d_inode(it->dentry);
 	struct erofs_vnode *const vi = EROFS_V(inode);
-	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
+	struct super_block *const sb = inode->i_sb;
+	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 	unsigned i;
 	int ret = 0;
 
@@ -537,7 +542,7 @@ static int shared_listxattr(struct listxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page(inode->i_sb,
+			it->it.page = erofs_get_meta_page_nofail(sb,
 				blkaddr, false);
 			BUG_ON(IS_ERR(it->it.page));
 			it->it.kaddr = kmap_atomic(it->it.page);
-- 
1.9.1

[PREVIEW] [PATCH v3 2/3] staging: erofs: separate erofs_get_meta_page

[Chao Yu]

On 2018/8/11 0:48, Gao Xiang wrote:
> This patch separates 'erofs_get_meta_page' into 'erofs_get_meta_page'
> and 'erofs_get_meta_page_nofail'. The second one ensures that it
> should not fail under memory pressure and should make best efforts
> if IO errors occur.
> 
> It also adds const variables in order to fulfill 80 character limit.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>

Reviewed-by: Chao Yu <yuchao0 at huawei.com>

Thanks,

[PREVIEW] [PATCH v3 2/3] staging: erofs: separate erofs_get_meta_page

[Chao Yu]

On 2018/8/11 0:48, Gao Xiang wrote:
> This patch separates 'erofs_get_meta_page' into 'erofs_get_meta_page'
> and 'erofs_get_meta_page_nofail'. The second one ensures that it
> should not fail under memory pressure and should make best efforts
> if IO errors occur.
> 
> It also adds const variables in order to fulfill 80 character limit.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
> ---
> change log v3:
>  - add EROFS_FS_IO_MAX_RETRIES to handle IO Error for nofail cases
>    according to Chao's suggestion;
>  - remove all 'unlikely' hints together with IS_ERR.
> 
>  drivers/staging/erofs/Kconfig     |  9 +++++++
>  drivers/staging/erofs/data.c      | 52 +++++++++++++++++++++++++--------------
>  drivers/staging/erofs/internal.h  | 24 ++++++++++++++----
>  drivers/staging/erofs/unzip_vle.c | 12 +++++----
>  drivers/staging/erofs/xattr.c     | 23 ++++++++++-------
>  5 files changed, 83 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/erofs/Kconfig b/drivers/staging/erofs/Kconfig
> index 96f6149..7f209b3 100644
> --- a/drivers/staging/erofs/Kconfig
> +++ b/drivers/staging/erofs/Kconfig
> @@ -78,6 +78,15 @@ config EROFS_FAULT_INJECTION
>  	  Test EROFS to inject faults such as ENOMEM, EIO, and so on.
>  	  If unsure, say N.
>  
> +config EROFS_FS_IO_MAX_RETRIES
> +	int "EROFS IO Maximum Retries"
> +	depends on EROFS_FS
> +	default "5"
> +	help
> +	  Maximum retry count of IO Errors.
> +
> +	  If unsure, leave the default value (5 retries, 6 IOs at most).
> +
>  config EROFS_FS_ZIP
>  	bool "EROFS Data Compresssion Support"
>  	depends on EROFS_FS
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index e0c046d..908b774 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -39,31 +39,44 @@ static inline void read_endio(struct bio *bio)
>  }
>  
>  /* prio -- true is used for dir */
> -struct page *erofs_get_meta_page(struct super_block *sb,
> -	erofs_blk_t blkaddr, bool prio)
> +struct page *__erofs_get_meta_page(struct super_block *sb,
> +	erofs_blk_t blkaddr, bool prio, bool nofail)
>  {
> -	struct inode *bd_inode = sb->s_bdev->bd_inode;
> -	struct address_space *mapping = bd_inode->i_mapping;
> +	struct inode *const bd_inode = sb->s_bdev->bd_inode;
> +	struct address_space *const mapping = bd_inode->i_mapping;
> +	/* prefer retrying in the allocator to blindly looping below */
> +	const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
> +		(nofail ? __GFP_NOFAIL : 0);
> +	unsigned int io_retries = nofail ? CONFIG_EROFS_FS_IO_MAX_RETRIES : 0;

I failed to compile as below:

/home/yuchao/git/erofs/data.c: In function ?__erofs_get_meta_page?:
/home/yuchao/git/erofs/data.c:50:37: error: ?CONFIG_EROFS_FS_IO_MAX_RETRIES?
undeclared (first use in this function)
  unsigned int io_retries = nofail ? CONFIG_EROFS_FS_IO_MAX_RETRIES : 0;
                                     ^
/home/yuchao/git/erofs/data.c:50:37: note: each undeclared identifier is
reported only once for each function it appears in

It's better to change as below?

@@ -47,9 +47,14 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
        /* prefer retrying in the allocator to blindly looping below */
        const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
                (nofail ? __GFP_NOFAIL : 0);
-       unsigned int io_retries = nofail ? CONFIG_EROFS_FS_IO_MAX_RETRIES : 0;
+       unsigned int io_retries = 0;
        struct page *page;

+#ifdef CONFIG_EROFS_FS_IO_MAX_RETRIES
+       if (nofail)
+               io_retires = CONFIG_EROFS_FS_IO_MAX_RETRIES;
+#endif
+

Thanks,

>  	struct page *page;
>  
>  repeat:
> -	page = find_or_create_page(mapping, blkaddr,
> -	/*
> -	 * Prefer looping in the allocator rather than here,
> -	 * at least that code knows what it's doing.
> -	 */
> -		mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL);
> -
> -	BUG_ON(!page || !PageLocked(page));
> +	page = find_or_create_page(mapping, blkaddr, gfp);
> +	if (unlikely(page == NULL)) {
> +		DBG_BUGON(nofail);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	DBG_BUGON(!PageLocked(page));
>  
>  	if (!PageUptodate(page)) {
>  		struct bio *bio;
>  		int err;
>  
> -		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
> +		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, nofail);
> +		if (unlikely(bio == NULL)) {
> +			DBG_BUGON(nofail);
> +			err = -ENOMEM;
> +err_out:
> +			unlock_page(page);
> +			put_page(page);
> +			return ERR_PTR(err);
> +		}
>  
>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
> -		BUG_ON(err != PAGE_SIZE);
> +		if (unlikely(err != PAGE_SIZE)) {
> +			err = -EFAULT;
> +			goto err_out;
> +		}
>  
>  		__submit_bio(bio, REQ_OP_READ,
>  			REQ_META | (prio ? REQ_PRIO : 0));
> @@ -72,6 +85,7 @@ struct page *erofs_get_meta_page(struct super_block *sb,
>  
>  		/* the page has been truncated by others? */
>  		if (unlikely(page->mapping != mapping)) {
> +unlock_repeat:
>  			unlock_page(page);
>  			put_page(page);
>  			goto repeat;
> @@ -79,10 +93,12 @@ struct page *erofs_get_meta_page(struct super_block *sb,
>  
>  		/* more likely a read error */
>  		if (unlikely(!PageUptodate(page))) {
> -			unlock_page(page);
> -			put_page(page);
> -
> -			page = ERR_PTR(-EIO);
> +			if (io_retries) {
> +				--io_retries;
> +				goto unlock_repeat;
> +			}
> +			err = -EIO;
> +			goto err_out;
>  		}
>  	}
>  	return page;
> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
> index 79d4e08..ecb6c02 100644
> --- a/drivers/staging/erofs/internal.h
> +++ b/drivers/staging/erofs/internal.h
> @@ -453,8 +453,21 @@ static inline void __submit_bio(struct bio *bio, unsigned op, unsigned op_flags)
>  	submit_bio(bio);
>  }
>  
> -extern struct page *erofs_get_meta_page(struct super_block *sb,
> -	erofs_blk_t blkaddr, bool prio);
> +extern struct page *__erofs_get_meta_page(struct super_block *sb,
> +	erofs_blk_t blkaddr, bool prio, bool nofail);
> +
> +static inline struct page *erofs_get_meta_page(struct super_block *sb,
> +	erofs_blk_t blkaddr, bool prio)
> +{
> +	return __erofs_get_meta_page(sb, blkaddr, prio, false);
> +}
> +
> +static inline struct page *erofs_get_meta_page_nofail(struct super_block *sb,
> +	erofs_blk_t blkaddr, bool prio)
> +{
> +	return __erofs_get_meta_page(sb, blkaddr, prio, true);
> +}
> +
>  extern int erofs_map_blocks(struct inode *, struct erofs_map_blocks *, int);
>  extern int erofs_map_blocks_iter(struct inode *, struct erofs_map_blocks *,
>  	struct page **, int);
> @@ -465,10 +478,11 @@ struct erofs_map_blocks_iter {
>  };
>  
>  
> -static inline struct page *erofs_get_inline_page(struct inode *inode,
> -	erofs_blk_t blkaddr)
> +static inline struct page *
> +erofs_get_inline_page_nofail(struct inode *inode,
> +			     erofs_blk_t blkaddr)
>  {
> -	return erofs_get_meta_page(inode->i_sb,
> +	return erofs_get_meta_page_nofail(inode->i_sb,
>  		blkaddr, S_ISDIR(inode->i_mode));
>  }
>  
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index 6bb7aed..d7692a2 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -1488,7 +1488,8 @@ static erofs_off_t vle_get_logical_extent_head(
>  	erofs_blk_t blkaddr = vle_extent_blkaddr(inode, lcn);
>  	struct z_erofs_vle_decompressed_index *di;
>  	unsigned long long ofs;
> -	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
> +	struct super_block *const sb = inode->i_sb;
> +	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
>  	const unsigned int clustersize = 1 << clusterbits;
>  
>  	if (page->index != blkaddr) {
> @@ -1496,8 +1497,8 @@ static erofs_off_t vle_get_logical_extent_head(
>  		unlock_page(page);
>  		put_page(page);
>  
> -		*page_iter = page = erofs_get_meta_page(inode->i_sb,
> -			blkaddr, false);
> +		page = erofs_get_meta_page_nofail(sb, blkaddr, false);
> +		*page_iter = page;
>  		*kaddr_iter = kmap_atomic(page);
>  	}
>  
> @@ -1538,7 +1539,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>  	struct page *mpage = *mpage_ret;
>  	void *kaddr;
>  	bool initial;
> -	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
> +	struct super_block *const sb = inode->i_sb;
> +	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
>  	const unsigned int clustersize = 1 << clusterbits;
>  	int err = 0;
>  
> @@ -1569,7 +1571,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
>  		if (mpage != NULL)
>  			put_page(mpage);
>  
> -		mpage = erofs_get_meta_page(inode->i_sb, e_blkaddr, false);
> +		mpage = erofs_get_meta_page_nofail(sb, e_blkaddr, false);
>  		*mpage_ret = mpage;
>  	} else {
>  		lock_page(mpage);
> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
> index 0e9cfec..2593c85 100644
> --- a/drivers/staging/erofs/xattr.c
> +++ b/drivers/staging/erofs/xattr.c
> @@ -38,6 +38,7 @@ static void init_inode_xattrs(struct inode *inode)
>  	struct xattr_iter it;
>  	unsigned i;
>  	struct erofs_xattr_ibody_header *ih;
> +	struct super_block *sb;
>  	struct erofs_sb_info *sbi;
>  	struct erofs_vnode *vi;
>  	bool atomic_map;
> @@ -48,11 +49,12 @@ static void init_inode_xattrs(struct inode *inode)
>  	vi = EROFS_V(inode);
>  	BUG_ON(!vi->xattr_isize);
>  
> -	sbi = EROFS_I_SB(inode);
> +	sb = inode->i_sb;
> +	sbi = EROFS_SB(sb);
>  	it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
>  	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>  
> -	it.page = erofs_get_inline_page(inode, it.blkaddr);
> +	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
>  	BUG_ON(IS_ERR(it.page));
>  
>  	/* read in shared xattr array (non-atomic, see kmalloc below) */
> @@ -75,7 +77,7 @@ static void init_inode_xattrs(struct inode *inode)
>  			BUG_ON(it.ofs != EROFS_BLKSIZ);
>  			xattr_iter_end(&it, atomic_map);
>  
> -			it.page = erofs_get_meta_page(inode->i_sb,
> +			it.page = erofs_get_meta_page_nofail(sb,
>  				++it.blkaddr, S_ISDIR(inode->i_mode));
>  			BUG_ON(IS_ERR(it.page));
>  
> @@ -105,7 +107,8 @@ static void xattr_iter_fixup(struct xattr_iter *it)
>  		xattr_iter_end(it, true);
>  
>  		it->blkaddr += erofs_blknr(it->ofs);
> -		it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
> +		it->page = erofs_get_meta_page_nofail(it->sb,
> +			it->blkaddr, false);
>  		BUG_ON(IS_ERR(it->page));
>  
>  		it->kaddr = kmap_atomic(it->page);
> @@ -131,7 +134,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
>  	it->blkaddr = erofs_blknr(iloc(sbi, vi->nid) + inline_xattr_ofs);
>  	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
>  
> -	it->page = erofs_get_inline_page(inode, it->blkaddr);
> +	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
>  	BUG_ON(IS_ERR(it->page));
>  	it->kaddr = kmap_atomic(it->page);
>  
> @@ -300,7 +303,8 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
>  static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>  {
>  	struct erofs_vnode *const vi = EROFS_V(inode);
> -	struct erofs_sb_info *const sbi = EROFS_SB(inode->i_sb);
> +	struct super_block *const sb = inode->i_sb;
> +	struct erofs_sb_info *const sbi = EROFS_SB(sb);
>  	unsigned i;
>  	int ret = -ENOATTR;
>  
> @@ -314,7 +318,7 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>  			if (i)
>  				xattr_iter_end(&it->it, true);
>  
> -			it->it.page = erofs_get_meta_page(inode->i_sb,
> +			it->it.page = erofs_get_meta_page_nofail(sb,
>  				blkaddr, false);
>  			BUG_ON(IS_ERR(it->it.page));
>  			it->it.kaddr = kmap_atomic(it->it.page);
> @@ -524,7 +528,8 @@ static int shared_listxattr(struct listxattr_iter *it)
>  {
>  	struct inode *const inode = d_inode(it->dentry);
>  	struct erofs_vnode *const vi = EROFS_V(inode);
> -	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
> +	struct super_block *const sb = inode->i_sb;
> +	struct erofs_sb_info *const sbi = EROFS_SB(sb);
>  	unsigned i;
>  	int ret = 0;
>  
> @@ -537,7 +542,7 @@ static int shared_listxattr(struct listxattr_iter *it)
>  			if (i)
>  				xattr_iter_end(&it->it, true);
>  
> -			it->it.page = erofs_get_meta_page(inode->i_sb,
> +			it->it.page = erofs_get_meta_page_nofail(sb,
>  				blkaddr, false);
>  			BUG_ON(IS_ERR(it->it.page));
>  			it->it.kaddr = kmap_atomic(it->it.page);
> 

[PREVIEW] [PATCH v4 2/3] staging: erofs: separate erofs_get_meta_page

[Gao Xiang]

From: Gao Xiang <gaoxiang25@huawei.com>

This patch separates 'erofs_get_meta_page' into 'erofs_get_meta_page'
and 'erofs_get_meta_page_nofail'. The second one ensures that it
should not fail under memory pressure and should make best efforts
if IO errors occur.

It also adds auxiliary variables in order to fulfill 80 character limit.

Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
change log v4:
 - introduce EROFS_IO_MAX_RETRIES_NOFAIL to decouple from Kconfig

change log v3:
 - add EROFS_FS_IO_MAX_RETRIES to handle IO Error for nofail cases
   according to Chao's suggestion;
 - remove all 'unlikely' hints together with IS_ERR.

 drivers/staging/erofs/Kconfig     |  9 +++++++
 drivers/staging/erofs/data.c      | 52 +++++++++++++++++++++++++--------------
 drivers/staging/erofs/internal.h  | 30 ++++++++++++++++++----
 drivers/staging/erofs/unzip_vle.c | 12 +++++----
 drivers/staging/erofs/xattr.c     | 23 ++++++++++-------
 5 files changed, 89 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/erofs/Kconfig b/drivers/staging/erofs/Kconfig
index 96f6149..7f209b3 100644
--- a/drivers/staging/erofs/Kconfig
+++ b/drivers/staging/erofs/Kconfig
@@ -78,6 +78,15 @@ config EROFS_FAULT_INJECTION
 	  Test EROFS to inject faults such as ENOMEM, EIO, and so on.
 	  If unsure, say N.
 
+config EROFS_FS_IO_MAX_RETRIES
+	int "EROFS IO Maximum Retries"
+	depends on EROFS_FS
+	default "5"
+	help
+	  Maximum retry count of IO Errors.
+
+	  If unsure, leave the default value (5 retries, 6 IOs at most).
+
 config EROFS_FS_ZIP
 	bool "EROFS Data Compresssion Support"
 	depends on EROFS_FS
diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index e0c046d..0570af5 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -39,31 +39,44 @@ static inline void read_endio(struct bio *bio)
 }
 
 /* prio -- true is used for dir */
-struct page *erofs_get_meta_page(struct super_block *sb,
-	erofs_blk_t blkaddr, bool prio)
+struct page *__erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio, bool nofail)
 {
-	struct inode *bd_inode = sb->s_bdev->bd_inode;
-	struct address_space *mapping = bd_inode->i_mapping;
+	struct inode *const bd_inode = sb->s_bdev->bd_inode;
+	struct address_space *const mapping = bd_inode->i_mapping;
+	/* prefer retrying in the allocator to blindly looping below */
+	const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
+		(nofail ? __GFP_NOFAIL : 0);
+	unsigned int io_retries = nofail ? EROFS_IO_MAX_RETRIES_NOFAIL : 0;
 	struct page *page;
 
 repeat:
-	page = find_or_create_page(mapping, blkaddr,
-	/*
-	 * Prefer looping in the allocator rather than here,
-	 * at least that code knows what it's doing.
-	 */
-		mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL);
-
-	BUG_ON(!page || !PageLocked(page));
+	page = find_or_create_page(mapping, blkaddr, gfp);
+	if (unlikely(page == NULL)) {
+		DBG_BUGON(nofail);
+		return ERR_PTR(-ENOMEM);
+	}
+	DBG_BUGON(!PageLocked(page));
 
 	if (!PageUptodate(page)) {
 		struct bio *bio;
 		int err;
 
-		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, true);
+		bio = erofs_grab_bio(sb, blkaddr, 1, read_endio, nofail);
+		if (unlikely(bio == NULL)) {
+			DBG_BUGON(nofail);
+			err = -ENOMEM;
+err_out:
+			unlock_page(page);
+			put_page(page);
+			return ERR_PTR(err);
+		}
 
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
-		BUG_ON(err != PAGE_SIZE);
+		if (unlikely(err != PAGE_SIZE)) {
+			err = -EFAULT;
+			goto err_out;
+		}
 
 		__submit_bio(bio, REQ_OP_READ,
 			REQ_META | (prio ? REQ_PRIO : 0));
@@ -72,6 +85,7 @@ struct page *erofs_get_meta_page(struct super_block *sb,
 
 		/* the page has been truncated by others? */
 		if (unlikely(page->mapping != mapping)) {
+unlock_repeat:
 			unlock_page(page);
 			put_page(page);
 			goto repeat;
@@ -79,10 +93,12 @@ struct page *erofs_get_meta_page(struct super_block *sb,
 
 		/* more likely a read error */
 		if (unlikely(!PageUptodate(page))) {
-			unlock_page(page);
-			put_page(page);
-
-			page = ERR_PTR(-EIO);
+			if (io_retries) {
+				--io_retries;
+				goto unlock_repeat;
+			}
+			err = -EIO;
+			goto err_out;
 		}
 	}
 	return page;
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 1353b3f..a756abe 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -453,8 +453,27 @@ static inline void __submit_bio(struct bio *bio, unsigned op, unsigned op_flags)
 	submit_bio(bio);
 }
 
-extern struct page *erofs_get_meta_page(struct super_block *sb,
-	erofs_blk_t blkaddr, bool prio);
+#ifndef CONFIG_EROFS_FS_IO_MAX_RETRIES
+#define EROFS_IO_MAX_RETRIES_NOFAIL	0
+#else
+#define EROFS_IO_MAX_RETRIES_NOFAIL	CONFIG_EROFS_FS_IO_MAX_RETRIES
+#endif
+
+extern struct page *__erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio, bool nofail);
+
+static inline struct page *erofs_get_meta_page(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio)
+{
+	return __erofs_get_meta_page(sb, blkaddr, prio, false);
+}
+
+static inline struct page *erofs_get_meta_page_nofail(struct super_block *sb,
+	erofs_blk_t blkaddr, bool prio)
+{
+	return __erofs_get_meta_page(sb, blkaddr, prio, true);
+}
+
 extern int erofs_map_blocks(struct inode *, struct erofs_map_blocks *, int);
 extern int erofs_map_blocks_iter(struct inode *, struct erofs_map_blocks *,
 	struct page **, int);
@@ -465,10 +484,11 @@ struct erofs_map_blocks_iter {
 };
 
 
-static inline struct page *erofs_get_inline_page(struct inode *inode,
-	erofs_blk_t blkaddr)
+static inline struct page *
+erofs_get_inline_page_nofail(struct inode *inode,
+			     erofs_blk_t blkaddr)
 {
-	return erofs_get_meta_page(inode->i_sb,
+	return erofs_get_meta_page_nofail(inode->i_sb,
 		blkaddr, S_ISDIR(inode->i_mode));
 }
 
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 375c171..b2e05e2 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1488,7 +1488,8 @@ static erofs_off_t vle_get_logical_extent_head(
 	erofs_blk_t blkaddr = vle_extent_blkaddr(inode, lcn);
 	struct z_erofs_vle_decompressed_index *di;
 	unsigned long long ofs;
-	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
+	struct super_block *const sb = inode->i_sb;
+	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
 	const unsigned int clustersize = 1 << clusterbits;
 
 	if (page->index != blkaddr) {
@@ -1496,8 +1497,8 @@ static erofs_off_t vle_get_logical_extent_head(
 		unlock_page(page);
 		put_page(page);
 
-		*page_iter = page = erofs_get_meta_page(inode->i_sb,
-			blkaddr, false);
+		page = erofs_get_meta_page_nofail(sb, blkaddr, false);
+		*page_iter = page;
 		*kaddr_iter = kmap_atomic(page);
 	}
 
@@ -1538,7 +1539,8 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 	struct page *mpage = *mpage_ret;
 	void *kaddr;
 	bool initial;
-	const unsigned int clusterbits = EROFS_SB(inode->i_sb)->clusterbits;
+	struct super_block *const sb = inode->i_sb;
+	const unsigned int clusterbits = EROFS_SB(sb)->clusterbits;
 	const unsigned int clustersize = 1 << clusterbits;
 	int err = 0;
 
@@ -1569,7 +1571,7 @@ int z_erofs_map_blocks_iter(struct inode *inode,
 		if (mpage != NULL)
 			put_page(mpage);
 
-		mpage = erofs_get_meta_page(inode->i_sb, e_blkaddr, false);
+		mpage = erofs_get_meta_page_nofail(sb, e_blkaddr, false);
 		*mpage_ret = mpage;
 	} else {
 		lock_page(mpage);
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 0e9cfec..2593c85 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -38,6 +38,7 @@ static void init_inode_xattrs(struct inode *inode)
 	struct xattr_iter it;
 	unsigned i;
 	struct erofs_xattr_ibody_header *ih;
+	struct super_block *sb;
 	struct erofs_sb_info *sbi;
 	struct erofs_vnode *vi;
 	bool atomic_map;
@@ -48,11 +49,12 @@ static void init_inode_xattrs(struct inode *inode)
 	vi = EROFS_V(inode);
 	BUG_ON(!vi->xattr_isize);
 
-	sbi = EROFS_I_SB(inode);
+	sb = inode->i_sb;
+	sbi = EROFS_SB(sb);
 	it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
 	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
 
-	it.page = erofs_get_inline_page(inode, it.blkaddr);
+	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
 	BUG_ON(IS_ERR(it.page));
 
 	/* read in shared xattr array (non-atomic, see kmalloc below) */
@@ -75,7 +77,7 @@ static void init_inode_xattrs(struct inode *inode)
 			BUG_ON(it.ofs != EROFS_BLKSIZ);
 			xattr_iter_end(&it, atomic_map);
 
-			it.page = erofs_get_meta_page(inode->i_sb,
+			it.page = erofs_get_meta_page_nofail(sb,
 				++it.blkaddr, S_ISDIR(inode->i_mode));
 			BUG_ON(IS_ERR(it.page));
 
@@ -105,7 +107,8 @@ static void xattr_iter_fixup(struct xattr_iter *it)
 		xattr_iter_end(it, true);
 
 		it->blkaddr += erofs_blknr(it->ofs);
-		it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
+		it->page = erofs_get_meta_page_nofail(it->sb,
+			it->blkaddr, false);
 		BUG_ON(IS_ERR(it->page));
 
 		it->kaddr = kmap_atomic(it->page);
@@ -131,7 +134,7 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
 	it->blkaddr = erofs_blknr(iloc(sbi, vi->nid) + inline_xattr_ofs);
 	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
 
-	it->page = erofs_get_inline_page(inode, it->blkaddr);
+	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
 	BUG_ON(IS_ERR(it->page));
 	it->kaddr = kmap_atomic(it->page);
 
@@ -300,7 +303,8 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
 static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 {
 	struct erofs_vnode *const vi = EROFS_V(inode);
-	struct erofs_sb_info *const sbi = EROFS_SB(inode->i_sb);
+	struct super_block *const sb = inode->i_sb;
+	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 	unsigned i;
 	int ret = -ENOATTR;
 
@@ -314,7 +318,7 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page(inode->i_sb,
+			it->it.page = erofs_get_meta_page_nofail(sb,
 				blkaddr, false);
 			BUG_ON(IS_ERR(it->it.page));
 			it->it.kaddr = kmap_atomic(it->it.page);
@@ -524,7 +528,8 @@ static int shared_listxattr(struct listxattr_iter *it)
 {
 	struct inode *const inode = d_inode(it->dentry);
 	struct erofs_vnode *const vi = EROFS_V(inode);
-	struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
+	struct super_block *const sb = inode->i_sb;
+	struct erofs_sb_info *const sbi = EROFS_SB(sb);
 	unsigned i;
 	int ret = 0;
 
@@ -537,7 +542,7 @@ static int shared_listxattr(struct listxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page(inode->i_sb,
+			it->it.page = erofs_get_meta_page_nofail(sb,
 				blkaddr, false);
 			BUG_ON(IS_ERR(it->it.page));
 			it->it.kaddr = kmap_atomic(it->it.page);
-- 
2.7.4

[PREVIEW] [PATCH v3 2/3] staging: erofs: separate erofs_get_meta_page

[Gao Xiang]

Hi Chao,

On 2018/8/12 18:54, Chao Yu wrote:
> On 2018/8/11 0:48, Gao Xiang wrote:
>> This patch separates 'erofs_get_meta_page' into 'erofs_get_meta_page'
>> and 'erofs_get_meta_page_nofail'. The second one ensures that it
>> should not fail under memory pressure and should make best efforts
>> if IO errors occur.
>>
>> It also adds const variables in order to fulfill 80 character limit.
>>
>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>> ---
>> change log v3:
>>  - add EROFS_FS_IO_MAX_RETRIES to handle IO Error for nofail cases
>>    according to Chao's suggestion;
>>  - remove all 'unlikely' hints together with IS_ERR.
>>
>>  drivers/staging/erofs/Kconfig     |  9 +++++++
>>  drivers/staging/erofs/data.c      | 52 +++++++++++++++++++++++++--------------
>>  drivers/staging/erofs/internal.h  | 24 ++++++++++++++----
>>  drivers/staging/erofs/unzip_vle.c | 12 +++++----
>>  drivers/staging/erofs/xattr.c     | 23 ++++++++++-------
>>  5 files changed, 83 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/Kconfig b/drivers/staging/erofs/Kconfig
>> index 96f6149..7f209b3 100644
>> --- a/drivers/staging/erofs/Kconfig
>> +++ b/drivers/staging/erofs/Kconfig
>> @@ -78,6 +78,15 @@ config EROFS_FAULT_INJECTION
>>  	  Test EROFS to inject faults such as ENOMEM, EIO, and so on.
>>  	  If unsure, say N.
>>  
>> +config EROFS_FS_IO_MAX_RETRIES
>> +	int "EROFS IO Maximum Retries"
>> +	depends on EROFS_FS
>> +	default "5"
>> +	help
>> +	  Maximum retry count of IO Errors.
>> +
>> +	  If unsure, leave the default value (5 retries, 6 IOs at most).
>> +
>>  config EROFS_FS_ZIP
>>  	bool "EROFS Data Compresssion Support"
>>  	depends on EROFS_FS
>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>> index e0c046d..908b774 100644
>> --- a/drivers/staging/erofs/data.c
>> +++ b/drivers/staging/erofs/data.c
>> @@ -39,31 +39,44 @@ static inline void read_endio(struct bio *bio)
>>  }
>>  
>>  /* prio -- true is used for dir */
>> -struct page *erofs_get_meta_page(struct super_block *sb,
>> -	erofs_blk_t blkaddr, bool prio)
>> +struct page *__erofs_get_meta_page(struct super_block *sb,
>> +	erofs_blk_t blkaddr, bool prio, bool nofail)
>>  {
>> -	struct inode *bd_inode = sb->s_bdev->bd_inode;
>> -	struct address_space *mapping = bd_inode->i_mapping;
>> +	struct inode *const bd_inode = sb->s_bdev->bd_inode;
>> +	struct address_space *const mapping = bd_inode->i_mapping;
>> +	/* prefer retrying in the allocator to blindly looping below */
>> +	const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
>> +		(nofail ? __GFP_NOFAIL : 0);
>> +	unsigned int io_retries = nofail ? CONFIG_EROFS_FS_IO_MAX_RETRIES : 0;
> I failed to compile as below:
> 
> /home/yuchao/git/erofs/data.c: In function ?__erofs_get_meta_page?:
> /home/yuchao/git/erofs/data.c:50:37: error: ?CONFIG_EROFS_FS_IO_MAX_RETRIES?
> undeclared (first use in this function)
>   unsigned int io_retries = nofail ? CONFIG_EROFS_FS_IO_MAX_RETRIES : 0;
>                                      ^
> /home/yuchao/git/erofs/data.c:50:37: note: each undeclared identifier is
> reported only once for each function it appears in
> 
> It's better to change as below?
> 
> @@ -47,9 +47,14 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>         /* prefer retrying in the allocator to blindly looping below */
>         const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS) |
>                 (nofail ? __GFP_NOFAIL : 0);
> -       unsigned int io_retries = nofail ? CONFIG_EROFS_FS_IO_MAX_RETRIES : 0;
> +       unsigned int io_retries = 0;
>         struct page *page;
> 
> +#ifdef CONFIG_EROFS_FS_IO_MAX_RETRIES
> +       if (nofail)
> +               io_retires = CONFIG_EROFS_FS_IO_MAX_RETRIES;
> +#endif
> +
> 

Could you please check the patch I just sent? :)

Thanks,
Gao Xiang

> Thanks,
> 

[PREVIEW] [PATCH 4/4] staging: erofs: add error handling for xattr submodule

[Gao Xiang]

This patch enhances the missing error handling code for
xattr submodule, which improves the stability for the rare cases.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
 drivers/staging/erofs/xattr.c | 114 ++++++++++++++++++++++++++++--------------
 1 file changed, 77 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 702434b..bd911bb 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -24,16 +24,25 @@ struct xattr_iter {
 
 static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
 {
-	/* only init_inode_xattrs use non-atomic once */
+	/* the only user of kunmap() is 'init_inode_xattrs' */
 	if (unlikely(!atomic))
 		kunmap(it->page);
 	else
 		kunmap_atomic(it->kaddr);
+
 	unlock_page(it->page);
 	put_page(it->page);
 }
 
-static void init_inode_xattrs(struct inode *inode)
+static inline void xattr_iter_end_final(struct xattr_iter *it)
+{
+	if (unlikely(it->page == NULL))
+		return;
+
+	xattr_iter_end(it, true);
+}
+
+static int init_inode_xattrs(struct inode *inode)
 {
 	struct xattr_iter it;
 	unsigned i;
@@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
 	bool atomic_map;
 
 	if (likely(inode_has_inited_xattr(inode)))
-		return;
+		return 0;
 
 	vi = EROFS_V(inode);
 	BUG_ON(!vi->xattr_isize);
@@ -55,7 +64,8 @@ static void init_inode_xattrs(struct inode *inode)
 	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
 
 	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
-	BUG_ON(IS_ERR(it.page));
+	if (unlikely(IS_ERR(it.page)))
+		return PTR_ERR(it.page);
 
 	/* read in shared xattr array (non-atomic, see kmalloc below) */
 	it.kaddr = kmap(it.page);
@@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
 	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
 
 	vi->xattr_shared_count = ih->h_shared_count;
-	vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
-		vi->xattr_shared_count, sizeof(unsigned),
-		GFP_KERNEL | __GFP_NOFAIL);
+	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
+						sizeof(unsigned), GFP_KERNEL);
+	if (unlikely(vi->xattr_shared_xattrs == NULL)) {
+		xattr_iter_end(&it, atomic_map);
+		return -ENOMEM;
+	}
 
 	/* let's skip ibody header */
 	it.ofs += sizeof(struct erofs_xattr_ibody_header);
@@ -79,7 +92,8 @@ static void init_inode_xattrs(struct inode *inode)
 
 			it.page = erofs_get_meta_page_nofail(sb,
 				++it.blkaddr, S_ISDIR(inode->i_mode));
-			BUG_ON(IS_ERR(it.page));
+			if (unlikely(IS_ERR(it.page)))
+				return PTR_ERR(it.page);
 
 			it.kaddr = kmap_atomic(it.page);
 			atomic_map = true;
@@ -92,6 +106,7 @@ static void init_inode_xattrs(struct inode *inode)
 	xattr_iter_end(&it, atomic_map);
 
 	inode_set_inited_xattr(inode);
+	return 0;
 }
 
 struct xattr_iter_handlers {
@@ -101,19 +116,24 @@ struct xattr_iter_handlers {
 	void (*value)(struct xattr_iter *, unsigned, char *, unsigned);
 };
 
-static void xattr_iter_fixup(struct xattr_iter *it)
+static inline int xattr_iter_fixup(struct xattr_iter *it)
 {
-	if (unlikely(it->ofs >= EROFS_BLKSIZ)) {
-		xattr_iter_end(it, true);
+	if (likely(it->ofs < EROFS_BLKSIZ))
+		return 0;
 
-		it->blkaddr += erofs_blknr(it->ofs);
-		it->page = erofs_get_meta_page_nofail(it->sb,
-			it->blkaddr, false);
-		BUG_ON(IS_ERR(it->page));
+	xattr_iter_end(it, true);
 
-		it->kaddr = kmap_atomic(it->page);
-		it->ofs = erofs_blkoff(it->ofs);
+	it->blkaddr += erofs_blknr(it->ofs);
+
+	it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
+	if (unlikely(IS_ERR(it->page))) {
+		it->page = NULL;
+		return PTR_ERR(it->page);
 	}
+
+	it->kaddr = kmap_atomic(it->page);
+	it->ofs = erofs_blkoff(it->ofs);
+	return 0;
 }
 
 static int inline_xattr_iter_begin(struct xattr_iter *it,
@@ -135,21 +155,24 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
 	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
 
 	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
-	BUG_ON(IS_ERR(it->page));
-	it->kaddr = kmap_atomic(it->page);
+	if (unlikely(IS_ERR(it->page)))
+		return PTR_ERR(it->page);
 
+	it->kaddr = kmap_atomic(it->page);
 	return vi->xattr_isize - xattr_header_sz;
 }
 
 static int xattr_foreach(struct xattr_iter *it,
-	struct xattr_iter_handlers *op, unsigned *tlimit)
+	const struct xattr_iter_handlers *op, unsigned *tlimit)
 {
 	struct erofs_xattr_entry entry;
 	unsigned value_sz, processed, slice;
 	int err;
 
 	/* 0. fixup blkaddr, ofs, ipage */
-	xattr_iter_fixup(it);
+	err = xattr_iter_fixup(it);
+	if (unlikely(err))
+		return err;
 
 	/*
 	 * 1. read xattr entry to the memory,
@@ -181,7 +204,9 @@ static int xattr_foreach(struct xattr_iter *it,
 		if (it->ofs >= EROFS_BLKSIZ) {
 			BUG_ON(it->ofs > EROFS_BLKSIZ);
 
-			xattr_iter_fixup(it);
+			err = xattr_iter_fixup(it);
+			if (unlikely(err))
+				goto out;
 			it->ofs = 0;
 		}
 
@@ -213,7 +238,10 @@ static int xattr_foreach(struct xattr_iter *it,
 	while (processed < value_sz) {
 		if (it->ofs >= EROFS_BLKSIZ) {
 			BUG_ON(it->ofs > EROFS_BLKSIZ);
-			xattr_iter_fixup(it);
+
+			err = xattr_iter_fixup(it);
+			if (unlikely(err))
+				goto out;
 			it->ofs = 0;
 		}
 
@@ -273,7 +301,7 @@ static void xattr_copyvalue(struct xattr_iter *_it,
 	memcpy(it->buffer + processed, buf, len);
 }
 
-static struct xattr_iter_handlers find_xattr_handlers = {
+static const struct xattr_iter_handlers find_xattr_handlers = {
 	.entry = xattr_entrymatch,
 	.name = xattr_namematch,
 	.alloc_buffer = xattr_checkbuffer,
@@ -294,8 +322,11 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
 		if ((ret = xattr_foreach(&it->it,
 			&find_xattr_handlers, &remaining)) >= 0)
 			break;
+
+		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
+			break;
 	}
-	xattr_iter_end(&it->it, true);
+	xattr_iter_end_final(&it->it);
 
 	return ret < 0 ? ret : it->buffer_size;
 }
@@ -318,9 +349,10 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page_nofail(sb,
-				blkaddr, false);
-			BUG_ON(IS_ERR(it->it.page));
+			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
+			if (unlikely(IS_ERR(it->it.page)))
+				return PTR_ERR(it->it.page);
+
 			it->it.kaddr = kmap_atomic(it->it.page);
 			it->it.blkaddr = blkaddr;
 		}
@@ -328,9 +360,12 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 		if ((ret = xattr_foreach(&it->it,
 			&find_xattr_handlers, NULL)) >= 0)
 			break;
+
+		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
+			break;
 	}
 	if (vi->xattr_shared_count)
-		xattr_iter_end(&it->it, true);
+		xattr_iter_end_final(&it->it);
 
 	return ret < 0 ? ret : it->buffer_size;
 }
@@ -355,7 +390,9 @@ int erofs_getxattr(struct inode *inode, int index,
 	if (unlikely(name == NULL))
 		return -EINVAL;
 
-	init_inode_xattrs(inode);
+	ret = init_inode_xattrs(inode);
+	if (unlikely(ret < 0))
+		return ret;
 
 	it.index = index;
 
@@ -498,7 +535,7 @@ static int xattr_skipvalue(struct xattr_iter *_it,
 	return 1;
 }
 
-static struct xattr_iter_handlers list_xattr_handlers = {
+static const struct xattr_iter_handlers list_xattr_handlers = {
 	.entry = xattr_entrylist,
 	.name = xattr_namelist,
 	.alloc_buffer = xattr_skipvalue,
@@ -520,7 +557,7 @@ static int inline_listxattr(struct listxattr_iter *it)
 			&list_xattr_handlers, &remaining)) < 0)
 			break;
 	}
-	xattr_iter_end(&it->it, true);
+	xattr_iter_end_final(&it->it);
 	return ret < 0 ? ret : it->buffer_ofs;
 }
 
@@ -542,9 +579,10 @@ static int shared_listxattr(struct listxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page_nofail(sb,
-				blkaddr, false);
-			BUG_ON(IS_ERR(it->it.page));
+			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
+			if (unlikely(IS_ERR(it->it.page)))
+				return PTR_ERR(it->it.page);
+
 			it->it.kaddr = kmap_atomic(it->it.page);
 			it->it.blkaddr = blkaddr;
 		}
@@ -554,7 +592,7 @@ static int shared_listxattr(struct listxattr_iter *it)
 			break;
 	}
 	if (vi->xattr_shared_count)
-		xattr_iter_end(&it->it, true);
+		xattr_iter_end_final(&it->it);
 
 	return ret < 0 ? ret : it->buffer_ofs;
 }
@@ -565,7 +603,9 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 	int ret;
 	struct listxattr_iter it;
 
-	init_inode_xattrs(d_inode(dentry));
+	ret = init_inode_xattrs(d_inode(dentry));
+	if (unlikely(ret < 0))
+		return ret;
 
 	it.dentry = dentry;
 	it.buffer = buffer;
-- 
1.9.1

[PREVIEW] [PATCH 4/4] staging: erofs: add error handling for xattr submodule

[Chao Yu]

On 2018/8/1 14:01, Gao Xiang wrote:
> This patch enhances the missing error handling code for
> xattr submodule, which improves the stability for the rare cases.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
> ---
>  drivers/staging/erofs/xattr.c | 114 ++++++++++++++++++++++++++++--------------
>  1 file changed, 77 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
> index 702434b..bd911bb 100644
> --- a/drivers/staging/erofs/xattr.c
> +++ b/drivers/staging/erofs/xattr.c
> @@ -24,16 +24,25 @@ struct xattr_iter {
>  
>  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>  {
> -	/* only init_inode_xattrs use non-atomic once */
> +	/* the only user of kunmap() is 'init_inode_xattrs' */
>  	if (unlikely(!atomic))
>  		kunmap(it->page);
>  	else
>  		kunmap_atomic(it->kaddr);
> +
>  	unlock_page(it->page);
>  	put_page(it->page);
>  }
>  
> -static void init_inode_xattrs(struct inode *inode)
> +static inline void xattr_iter_end_final(struct xattr_iter *it)
> +{
> +	if (unlikely(it->page == NULL))
> +		return;
> +
> +	xattr_iter_end(it, true);
> +}
> +
> +static int init_inode_xattrs(struct inode *inode)
>  {
>  	struct xattr_iter it;
>  	unsigned i;
> @@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
>  	bool atomic_map;
>  
>  	if (likely(inode_has_inited_xattr(inode)))
> -		return;
> +		return 0;
>  
>  	vi = EROFS_V(inode);
>  	BUG_ON(!vi->xattr_isize);
> @@ -55,7 +64,8 @@ static void init_inode_xattrs(struct inode *inode)
>  	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>  
>  	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);

Need to introduce and use erofs_get_inline_page()?

> -	BUG_ON(IS_ERR(it.page));
> +	if (unlikely(IS_ERR(it.page)))
> +		return PTR_ERR(it.page);
>  
>  	/* read in shared xattr array (non-atomic, see kmalloc below) */
>  	it.kaddr = kmap(it.page);
> @@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
>  	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>  
>  	vi->xattr_shared_count = ih->h_shared_count;
> -	vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
> -		vi->xattr_shared_count, sizeof(unsigned),
> -		GFP_KERNEL | __GFP_NOFAIL);
> +	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
> +						sizeof(unsigned), GFP_KERNEL);
> +	if (unlikely(vi->xattr_shared_xattrs == NULL)) {
> +		xattr_iter_end(&it, atomic_map);
> +		return -ENOMEM;
> +	}
>  
>  	/* let's skip ibody header */
>  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
> @@ -79,7 +92,8 @@ static void init_inode_xattrs(struct inode *inode)
>  
>  			it.page = erofs_get_meta_page_nofail(sb,
>  				++it.blkaddr, S_ISDIR(inode->i_mode));

Need to use erofs_get_meta_page() instead?

Thanks,

> -			BUG_ON(IS_ERR(it.page));
> +			if (unlikely(IS_ERR(it.page)))
> +				return PTR_ERR(it.page);
>  
>  			it.kaddr = kmap_atomic(it.page);
>  			atomic_map = true;
> @@ -92,6 +106,7 @@ static void init_inode_xattrs(struct inode *inode)
>  	xattr_iter_end(&it, atomic_map);
>  
>  	inode_set_inited_xattr(inode);
> +	return 0;
>  }
>  
>  struct xattr_iter_handlers {
> @@ -101,19 +116,24 @@ struct xattr_iter_handlers {
>  	void (*value)(struct xattr_iter *, unsigned, char *, unsigned);
>  };
>  
> -static void xattr_iter_fixup(struct xattr_iter *it)
> +static inline int xattr_iter_fixup(struct xattr_iter *it)
>  {
> -	if (unlikely(it->ofs >= EROFS_BLKSIZ)) {
> -		xattr_iter_end(it, true);
> +	if (likely(it->ofs < EROFS_BLKSIZ))
> +		return 0;
>  
> -		it->blkaddr += erofs_blknr(it->ofs);
> -		it->page = erofs_get_meta_page_nofail(it->sb,
> -			it->blkaddr, false);
> -		BUG_ON(IS_ERR(it->page));
> +	xattr_iter_end(it, true);
>  
> -		it->kaddr = kmap_atomic(it->page);
> -		it->ofs = erofs_blkoff(it->ofs);
> +	it->blkaddr += erofs_blknr(it->ofs);
> +
> +	it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
> +	if (unlikely(IS_ERR(it->page))) {
> +		it->page = NULL;
> +		return PTR_ERR(it->page);
>  	}
> +
> +	it->kaddr = kmap_atomic(it->page);
> +	it->ofs = erofs_blkoff(it->ofs);
> +	return 0;
>  }
>  
>  static int inline_xattr_iter_begin(struct xattr_iter *it,
> @@ -135,21 +155,24 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
>  	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
>  
>  	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);

Ditto,

> -	BUG_ON(IS_ERR(it->page));
> -	it->kaddr = kmap_atomic(it->page);
> +	if (unlikely(IS_ERR(it->page)))
> +		return PTR_ERR(it->page);
>  
> +	it->kaddr = kmap_atomic(it->page);
>  	return vi->xattr_isize - xattr_header_sz;
>  }
>  
>  static int xattr_foreach(struct xattr_iter *it,
> -	struct xattr_iter_handlers *op, unsigned *tlimit)
> +	const struct xattr_iter_handlers *op, unsigned *tlimit)
>  {
>  	struct erofs_xattr_entry entry;
>  	unsigned value_sz, processed, slice;
>  	int err;
>  
>  	/* 0. fixup blkaddr, ofs, ipage */
> -	xattr_iter_fixup(it);
> +	err = xattr_iter_fixup(it);
> +	if (unlikely(err))
> +		return err;
>  
>  	/*
>  	 * 1. read xattr entry to the memory,
> @@ -181,7 +204,9 @@ static int xattr_foreach(struct xattr_iter *it,
>  		if (it->ofs >= EROFS_BLKSIZ) {
>  			BUG_ON(it->ofs > EROFS_BLKSIZ);
>  
> -			xattr_iter_fixup(it);
> +			err = xattr_iter_fixup(it);
> +			if (unlikely(err))
> +				goto out;
>  			it->ofs = 0;
>  		}
>  
> @@ -213,7 +238,10 @@ static int xattr_foreach(struct xattr_iter *it,
>  	while (processed < value_sz) {
>  		if (it->ofs >= EROFS_BLKSIZ) {
>  			BUG_ON(it->ofs > EROFS_BLKSIZ);
> -			xattr_iter_fixup(it);
> +
> +			err = xattr_iter_fixup(it);
> +			if (unlikely(err))
> +				goto out;
>  			it->ofs = 0;
>  		}
>  
> @@ -273,7 +301,7 @@ static void xattr_copyvalue(struct xattr_iter *_it,
>  	memcpy(it->buffer + processed, buf, len);
>  }
>  
> -static struct xattr_iter_handlers find_xattr_handlers = {
> +static const struct xattr_iter_handlers find_xattr_handlers = {
>  	.entry = xattr_entrymatch,
>  	.name = xattr_namematch,
>  	.alloc_buffer = xattr_checkbuffer,
> @@ -294,8 +322,11 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
>  		if ((ret = xattr_foreach(&it->it,
>  			&find_xattr_handlers, &remaining)) >= 0)
>  			break;
> +
> +		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
> +			break;
>  	}
> -	xattr_iter_end(&it->it, true);
> +	xattr_iter_end_final(&it->it);
>  
>  	return ret < 0 ? ret : it->buffer_size;
>  }
> @@ -318,9 +349,10 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>  			if (i)
>  				xattr_iter_end(&it->it, true);
>  
> -			it->it.page = erofs_get_meta_page_nofail(sb,
> -				blkaddr, false);
> -			BUG_ON(IS_ERR(it->it.page));
> +			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
> +			if (unlikely(IS_ERR(it->it.page)))
> +				return PTR_ERR(it->it.page);
> +
>  			it->it.kaddr = kmap_atomic(it->it.page);
>  			it->it.blkaddr = blkaddr;
>  		}
> @@ -328,9 +360,12 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>  		if ((ret = xattr_foreach(&it->it,
>  			&find_xattr_handlers, NULL)) >= 0)
>  			break;
> +
> +		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
> +			break;
>  	}
>  	if (vi->xattr_shared_count)
> -		xattr_iter_end(&it->it, true);
> +		xattr_iter_end_final(&it->it);
>  
>  	return ret < 0 ? ret : it->buffer_size;
>  }
> @@ -355,7 +390,9 @@ int erofs_getxattr(struct inode *inode, int index,
>  	if (unlikely(name == NULL))
>  		return -EINVAL;
>  
> -	init_inode_xattrs(inode);
> +	ret = init_inode_xattrs(inode);
> +	if (unlikely(ret < 0))
> +		return ret;
>  
>  	it.index = index;
>  
> @@ -498,7 +535,7 @@ static int xattr_skipvalue(struct xattr_iter *_it,
>  	return 1;
>  }
>  
> -static struct xattr_iter_handlers list_xattr_handlers = {
> +static const struct xattr_iter_handlers list_xattr_handlers = {
>  	.entry = xattr_entrylist,
>  	.name = xattr_namelist,
>  	.alloc_buffer = xattr_skipvalue,
> @@ -520,7 +557,7 @@ static int inline_listxattr(struct listxattr_iter *it)
>  			&list_xattr_handlers, &remaining)) < 0)
>  			break;
>  	}
> -	xattr_iter_end(&it->it, true);
> +	xattr_iter_end_final(&it->it);
>  	return ret < 0 ? ret : it->buffer_ofs;
>  }
>  
> @@ -542,9 +579,10 @@ static int shared_listxattr(struct listxattr_iter *it)
>  			if (i)
>  				xattr_iter_end(&it->it, true);
>  
> -			it->it.page = erofs_get_meta_page_nofail(sb,
> -				blkaddr, false);
> -			BUG_ON(IS_ERR(it->it.page));
> +			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
> +			if (unlikely(IS_ERR(it->it.page)))
> +				return PTR_ERR(it->it.page);
> +
>  			it->it.kaddr = kmap_atomic(it->it.page);
>  			it->it.blkaddr = blkaddr;
>  		}
> @@ -554,7 +592,7 @@ static int shared_listxattr(struct listxattr_iter *it)
>  			break;
>  	}
>  	if (vi->xattr_shared_count)
> -		xattr_iter_end(&it->it, true);
> +		xattr_iter_end_final(&it->it);
>  
>  	return ret < 0 ? ret : it->buffer_ofs;
>  }
> @@ -565,7 +603,9 @@ ssize_t erofs_listxattr(struct dentry *dentry,
>  	int ret;
>  	struct listxattr_iter it;
>  
> -	init_inode_xattrs(d_inode(dentry));
> +	ret = init_inode_xattrs(d_inode(dentry));
> +	if (unlikely(ret < 0))
> +		return ret;
>  
>  	it.dentry = dentry;
>  	it.buffer = buffer;
> 

[PREVIEW] [PATCH 4/4] staging: erofs: add error handling for xattr submodule

[Gao Xiang]

Hi Chao,

On 2018/8/10 11:40, Chao Yu wrote:
> On 2018/8/1 14:01, Gao Xiang wrote:
>> This patch enhances the missing error handling code for
>> xattr submodule, which improves the stability for the rare cases.
>>
>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>> ---
>>  drivers/staging/erofs/xattr.c | 114 ++++++++++++++++++++++++++++--------------
>>  1 file changed, 77 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
>> index 702434b..bd911bb 100644
>> --- a/drivers/staging/erofs/xattr.c
>> +++ b/drivers/staging/erofs/xattr.c
>> @@ -24,16 +24,25 @@ struct xattr_iter {
>>  
>>  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>>  {
>> -	/* only init_inode_xattrs use non-atomic once */
>> +	/* the only user of kunmap() is 'init_inode_xattrs' */
>>  	if (unlikely(!atomic))
>>  		kunmap(it->page);
>>  	else
>>  		kunmap_atomic(it->kaddr);
>> +
>>  	unlock_page(it->page);
>>  	put_page(it->page);
>>  }
>>  
>> -static void init_inode_xattrs(struct inode *inode)
>> +static inline void xattr_iter_end_final(struct xattr_iter *it)
>> +{
>> +	if (unlikely(it->page == NULL))
>> +		return;
>> +
>> +	xattr_iter_end(it, true);
>> +}
>> +
>> +static int init_inode_xattrs(struct inode *inode)
>>  {
>>  	struct xattr_iter it;
>>  	unsigned i;
>> @@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
>>  	bool atomic_map;
>>  
>>  	if (likely(inode_has_inited_xattr(inode)))
>> -		return;
>> +		return 0;
>>  
>>  	vi = EROFS_V(inode);
>>  	BUG_ON(!vi->xattr_isize);
>> @@ -55,7 +64,8 @@ static void init_inode_xattrs(struct inode *inode)
>>  	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>>  
>>  	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
> 
> Need to introduce and use erofs_get_inline_page()?
> 
>> -	BUG_ON(IS_ERR(it.page));
>> +	if (unlikely(IS_ERR(it.page)))
>> +		return PTR_ERR(it.page);
>>  
>>  	/* read in shared xattr array (non-atomic, see kmalloc below) */
>>  	it.kaddr = kmap(it.page);
>> @@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
>>  	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>>  
>>  	vi->xattr_shared_count = ih->h_shared_count;
>> -	vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
>> -		vi->xattr_shared_count, sizeof(unsigned),
>> -		GFP_KERNEL | __GFP_NOFAIL);
>> +	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>> +						sizeof(unsigned), GFP_KERNEL);
>> +	if (unlikely(vi->xattr_shared_xattrs == NULL)) {
>> +		xattr_iter_end(&it, atomic_map);
>> +		return -ENOMEM;
>> +	}
>>  
>>  	/* let's skip ibody header */
>>  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
>> @@ -79,7 +92,8 @@ static void init_inode_xattrs(struct inode *inode)
>>  
>>  			it.page = erofs_get_meta_page_nofail(sb,
>>  				++it.blkaddr, S_ISDIR(inode->i_mode));
> 
> Need to use erofs_get_meta_page() instead?
> 
> Thanks,

My consideration is for example,

init_inode_xattrs itself can be seen as a part of inode initialization (other fs does it in
inode initialization, but erofs does it in the first xattr request), if it fail, the only way
is to retry again...but with more overhead to retry...

How do you think about it? ...

> 
>> -			BUG_ON(IS_ERR(it.page));
>> +			if (unlikely(IS_ERR(it.page)))
>> +				return PTR_ERR(it.page);
>>  
>>  			it.kaddr = kmap_atomic(it.page);
>>  			atomic_map = true;
>> @@ -92,6 +106,7 @@ static void init_inode_xattrs(struct inode *inode)
>>  	xattr_iter_end(&it, atomic_map);
>>  
>>  	inode_set_inited_xattr(inode);
>> +	return 0;
>>  }
>>  
>>  struct xattr_iter_handlers {
>> @@ -101,19 +116,24 @@ struct xattr_iter_handlers {
>>  	void (*value)(struct xattr_iter *, unsigned, char *, unsigned);
>>  };
>>  
>> -static void xattr_iter_fixup(struct xattr_iter *it)
>> +static inline int xattr_iter_fixup(struct xattr_iter *it)
>>  {
>> -	if (unlikely(it->ofs >= EROFS_BLKSIZ)) {
>> -		xattr_iter_end(it, true);
>> +	if (likely(it->ofs < EROFS_BLKSIZ))
>> +		return 0;
>>  
>> -		it->blkaddr += erofs_blknr(it->ofs);
>> -		it->page = erofs_get_meta_page_nofail(it->sb,
>> -			it->blkaddr, false);
>> -		BUG_ON(IS_ERR(it->page));
>> +	xattr_iter_end(it, true);
>>  
>> -		it->kaddr = kmap_atomic(it->page);
>> -		it->ofs = erofs_blkoff(it->ofs);
>> +	it->blkaddr += erofs_blknr(it->ofs);
>> +
>> +	it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
>> +	if (unlikely(IS_ERR(it->page))) {
>> +		it->page = NULL;
>> +		return PTR_ERR(it->page);
>>  	}
>> +
>> +	it->kaddr = kmap_atomic(it->page);
>> +	it->ofs = erofs_blkoff(it->ofs);
>> +	return 0;
>>  }
>>  
>>  static int inline_xattr_iter_begin(struct xattr_iter *it,
>> @@ -135,21 +155,24 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
>>  	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
>>  
>>  	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
> 
> Ditto,
> 
This should be converted into erofs_get_inline_page actually... I'm lazy about that, sorry...
Let me send another patch..

Thanks,
Gao Xiang

>> -	BUG_ON(IS_ERR(it->page));
>> -	it->kaddr = kmap_atomic(it->page);
>> +	if (unlikely(IS_ERR(it->page)))
>> +		return PTR_ERR(it->page);
>>  
>> +	it->kaddr = kmap_atomic(it->page);
>>  	return vi->xattr_isize - xattr_header_sz;
>>  }
>>  
>>  static int xattr_foreach(struct xattr_iter *it,
>> -	struct xattr_iter_handlers *op, unsigned *tlimit)
>> +	const struct xattr_iter_handlers *op, unsigned *tlimit)
>>  {
>>  	struct erofs_xattr_entry entry;
>>  	unsigned value_sz, processed, slice;
>>  	int err;
>>  
>>  	/* 0. fixup blkaddr, ofs, ipage */
>> -	xattr_iter_fixup(it);
>> +	err = xattr_iter_fixup(it);
>> +	if (unlikely(err))
>> +		return err;
>>  
>>  	/*
>>  	 * 1. read xattr entry to the memory,
>> @@ -181,7 +204,9 @@ static int xattr_foreach(struct xattr_iter *it,
>>  		if (it->ofs >= EROFS_BLKSIZ) {
>>  			BUG_ON(it->ofs > EROFS_BLKSIZ);
>>  
>> -			xattr_iter_fixup(it);
>> +			err = xattr_iter_fixup(it);
>> +			if (unlikely(err))
>> +				goto out;
>>  			it->ofs = 0;
>>  		}
>>  
>> @@ -213,7 +238,10 @@ static int xattr_foreach(struct xattr_iter *it,
>>  	while (processed < value_sz) {
>>  		if (it->ofs >= EROFS_BLKSIZ) {
>>  			BUG_ON(it->ofs > EROFS_BLKSIZ);
>> -			xattr_iter_fixup(it);
>> +
>> +			err = xattr_iter_fixup(it);
>> +			if (unlikely(err))
>> +				goto out;
>>  			it->ofs = 0;
>>  		}
>>  
>> @@ -273,7 +301,7 @@ static void xattr_copyvalue(struct xattr_iter *_it,
>>  	memcpy(it->buffer + processed, buf, len);
>>  }
>>  
>> -static struct xattr_iter_handlers find_xattr_handlers = {
>> +static const struct xattr_iter_handlers find_xattr_handlers = {
>>  	.entry = xattr_entrymatch,
>>  	.name = xattr_namematch,
>>  	.alloc_buffer = xattr_checkbuffer,
>> @@ -294,8 +322,11 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
>>  		if ((ret = xattr_foreach(&it->it,
>>  			&find_xattr_handlers, &remaining)) >= 0)
>>  			break;
>> +
>> +		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
>> +			break;
>>  	}
>> -	xattr_iter_end(&it->it, true);
>> +	xattr_iter_end_final(&it->it);
>>  
>>  	return ret < 0 ? ret : it->buffer_size;
>>  }
>> @@ -318,9 +349,10 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>>  			if (i)
>>  				xattr_iter_end(&it->it, true);
>>  
>> -			it->it.page = erofs_get_meta_page_nofail(sb,
>> -				blkaddr, false);
>> -			BUG_ON(IS_ERR(it->it.page));
>> +			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
>> +			if (unlikely(IS_ERR(it->it.page)))
>> +				return PTR_ERR(it->it.page);
>> +
>>  			it->it.kaddr = kmap_atomic(it->it.page);
>>  			it->it.blkaddr = blkaddr;
>>  		}
>> @@ -328,9 +360,12 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>>  		if ((ret = xattr_foreach(&it->it,
>>  			&find_xattr_handlers, NULL)) >= 0)
>>  			break;
>> +
>> +		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
>> +			break;
>>  	}
>>  	if (vi->xattr_shared_count)
>> -		xattr_iter_end(&it->it, true);
>> +		xattr_iter_end_final(&it->it);
>>  
>>  	return ret < 0 ? ret : it->buffer_size;
>>  }
>> @@ -355,7 +390,9 @@ int erofs_getxattr(struct inode *inode, int index,
>>  	if (unlikely(name == NULL))
>>  		return -EINVAL;
>>  
>> -	init_inode_xattrs(inode);
>> +	ret = init_inode_xattrs(inode);
>> +	if (unlikely(ret < 0))
>> +		return ret;
>>  
>>  	it.index = index;
>>  
>> @@ -498,7 +535,7 @@ static int xattr_skipvalue(struct xattr_iter *_it,
>>  	return 1;
>>  }
>>  
>> -static struct xattr_iter_handlers list_xattr_handlers = {
>> +static const struct xattr_iter_handlers list_xattr_handlers = {
>>  	.entry = xattr_entrylist,
>>  	.name = xattr_namelist,
>>  	.alloc_buffer = xattr_skipvalue,
>> @@ -520,7 +557,7 @@ static int inline_listxattr(struct listxattr_iter *it)
>>  			&list_xattr_handlers, &remaining)) < 0)
>>  			break;
>>  	}
>> -	xattr_iter_end(&it->it, true);
>> +	xattr_iter_end_final(&it->it);
>>  	return ret < 0 ? ret : it->buffer_ofs;
>>  }
>>  
>> @@ -542,9 +579,10 @@ static int shared_listxattr(struct listxattr_iter *it)
>>  			if (i)
>>  				xattr_iter_end(&it->it, true);
>>  
>> -			it->it.page = erofs_get_meta_page_nofail(sb,
>> -				blkaddr, false);
>> -			BUG_ON(IS_ERR(it->it.page));
>> +			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
>> +			if (unlikely(IS_ERR(it->it.page)))
>> +				return PTR_ERR(it->it.page);
>> +
>>  			it->it.kaddr = kmap_atomic(it->it.page);
>>  			it->it.blkaddr = blkaddr;
>>  		}
>> @@ -554,7 +592,7 @@ static int shared_listxattr(struct listxattr_iter *it)
>>  			break;
>>  	}
>>  	if (vi->xattr_shared_count)
>> -		xattr_iter_end(&it->it, true);
>> +		xattr_iter_end_final(&it->it);
>>  
>>  	return ret < 0 ? ret : it->buffer_ofs;
>>  }
>> @@ -565,7 +603,9 @@ ssize_t erofs_listxattr(struct dentry *dentry,
>>  	int ret;
>>  	struct listxattr_iter it;
>>  
>> -	init_inode_xattrs(d_inode(dentry));
>> +	ret = init_inode_xattrs(d_inode(dentry));
>> +	if (unlikely(ret < 0))
>> +		return ret;
>>  
>>  	it.dentry = dentry;
>>  	it.buffer = buffer;
>>
> 

[PREVIEW] [PATCH 4/4] staging: erofs: add error handling for xattr submodule

[Chao Yu]

Hi Xiang,

On 2018/8/10 11:47, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/8/10 11:40, Chao Yu wrote:
>> On 2018/8/1 14:01, Gao Xiang wrote:
>>> This patch enhances the missing error handling code for
>>> xattr submodule, which improves the stability for the rare cases.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>> ---
>>>  drivers/staging/erofs/xattr.c | 114 ++++++++++++++++++++++++++++--------------
>>>  1 file changed, 77 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
>>> index 702434b..bd911bb 100644
>>> --- a/drivers/staging/erofs/xattr.c
>>> +++ b/drivers/staging/erofs/xattr.c
>>> @@ -24,16 +24,25 @@ struct xattr_iter {
>>>  
>>>  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>>>  {
>>> -	/* only init_inode_xattrs use non-atomic once */
>>> +	/* the only user of kunmap() is 'init_inode_xattrs' */
>>>  	if (unlikely(!atomic))
>>>  		kunmap(it->page);
>>>  	else
>>>  		kunmap_atomic(it->kaddr);
>>> +
>>>  	unlock_page(it->page);
>>>  	put_page(it->page);
>>>  }
>>>  
>>> -static void init_inode_xattrs(struct inode *inode)
>>> +static inline void xattr_iter_end_final(struct xattr_iter *it)
>>> +{
>>> +	if (unlikely(it->page == NULL))
>>> +		return;
>>> +
>>> +	xattr_iter_end(it, true);
>>> +}
>>> +
>>> +static int init_inode_xattrs(struct inode *inode)
>>>  {
>>>  	struct xattr_iter it;
>>>  	unsigned i;
>>> @@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
>>>  	bool atomic_map;
>>>  
>>>  	if (likely(inode_has_inited_xattr(inode)))
>>> -		return;
>>> +		return 0;
>>>  
>>>  	vi = EROFS_V(inode);
>>>  	BUG_ON(!vi->xattr_isize);
>>> @@ -55,7 +64,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>  	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>>>  
>>>  	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
>>
>> Need to introduce and use erofs_get_inline_page()?
>>
>>> -	BUG_ON(IS_ERR(it.page));
>>> +	if (unlikely(IS_ERR(it.page)))
>>> +		return PTR_ERR(it.page);
>>>  
>>>  	/* read in shared xattr array (non-atomic, see kmalloc below) */
>>>  	it.kaddr = kmap(it.page);
>>> @@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
>>>  	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>>>  
>>>  	vi->xattr_shared_count = ih->h_shared_count;
>>> -	vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
>>> -		vi->xattr_shared_count, sizeof(unsigned),
>>> -		GFP_KERNEL | __GFP_NOFAIL);
>>> +	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>>> +						sizeof(unsigned), GFP_KERNEL);
>>> +	if (unlikely(vi->xattr_shared_xattrs == NULL)) {
>>> +		xattr_iter_end(&it, atomic_map);
>>> +		return -ENOMEM;
>>> +	}
>>>  
>>>  	/* let's skip ibody header */
>>>  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
>>> @@ -79,7 +92,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>  
>>>  			it.page = erofs_get_meta_page_nofail(sb,
>>>  				++it.blkaddr, S_ISDIR(inode->i_mode));
>>
>> Need to use erofs_get_meta_page() instead?
>>
>> Thanks,
> 
> My consideration is for example,
> 
> init_inode_xattrs itself can be seen as a part of inode initialization (other fs does it in
> inode initialization, but erofs does it in the first xattr request), if it fail, the only way
> is to retry again...but with more overhead to retry...

I think we can easily return an error to user no matter we encounter -ENOMEM or
-EIO, so erofs_get_meta_page() is enough here, thoughts?

Thanks,

> 
> How do you think about it? ...
> 
>>
>>> -			BUG_ON(IS_ERR(it.page));
>>> +			if (unlikely(IS_ERR(it.page)))
>>> +				return PTR_ERR(it.page);
>>>  
>>>  			it.kaddr = kmap_atomic(it.page);
>>>  			atomic_map = true;
>>> @@ -92,6 +106,7 @@ static void init_inode_xattrs(struct inode *inode)
>>>  	xattr_iter_end(&it, atomic_map);
>>>  
>>>  	inode_set_inited_xattr(inode);
>>> +	return 0;
>>>  }
>>>  
>>>  struct xattr_iter_handlers {
>>> @@ -101,19 +116,24 @@ struct xattr_iter_handlers {
>>>  	void (*value)(struct xattr_iter *, unsigned, char *, unsigned);
>>>  };
>>>  
>>> -static void xattr_iter_fixup(struct xattr_iter *it)
>>> +static inline int xattr_iter_fixup(struct xattr_iter *it)
>>>  {
>>> -	if (unlikely(it->ofs >= EROFS_BLKSIZ)) {
>>> -		xattr_iter_end(it, true);
>>> +	if (likely(it->ofs < EROFS_BLKSIZ))
>>> +		return 0;
>>>  
>>> -		it->blkaddr += erofs_blknr(it->ofs);
>>> -		it->page = erofs_get_meta_page_nofail(it->sb,
>>> -			it->blkaddr, false);
>>> -		BUG_ON(IS_ERR(it->page));
>>> +	xattr_iter_end(it, true);
>>>  
>>> -		it->kaddr = kmap_atomic(it->page);
>>> -		it->ofs = erofs_blkoff(it->ofs);
>>> +	it->blkaddr += erofs_blknr(it->ofs);
>>> +
>>> +	it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
>>> +	if (unlikely(IS_ERR(it->page))) {
>>> +		it->page = NULL;
>>> +		return PTR_ERR(it->page);
>>>  	}
>>> +
>>> +	it->kaddr = kmap_atomic(it->page);
>>> +	it->ofs = erofs_blkoff(it->ofs);
>>> +	return 0;
>>>  }
>>>  
>>>  static int inline_xattr_iter_begin(struct xattr_iter *it,
>>> @@ -135,21 +155,24 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
>>>  	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
>>>  
>>>  	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
>>
>> Ditto,
>>
> This should be converted into erofs_get_inline_page actually... I'm lazy about that, sorry...
> Let me send another patch..
> 
> Thanks,
> Gao Xiang
> 
>>> -	BUG_ON(IS_ERR(it->page));
>>> -	it->kaddr = kmap_atomic(it->page);
>>> +	if (unlikely(IS_ERR(it->page)))
>>> +		return PTR_ERR(it->page);
>>>  
>>> +	it->kaddr = kmap_atomic(it->page);
>>>  	return vi->xattr_isize - xattr_header_sz;
>>>  }
>>>  
>>>  static int xattr_foreach(struct xattr_iter *it,
>>> -	struct xattr_iter_handlers *op, unsigned *tlimit)
>>> +	const struct xattr_iter_handlers *op, unsigned *tlimit)
>>>  {
>>>  	struct erofs_xattr_entry entry;
>>>  	unsigned value_sz, processed, slice;
>>>  	int err;
>>>  
>>>  	/* 0. fixup blkaddr, ofs, ipage */
>>> -	xattr_iter_fixup(it);
>>> +	err = xattr_iter_fixup(it);
>>> +	if (unlikely(err))
>>> +		return err;
>>>  
>>>  	/*
>>>  	 * 1. read xattr entry to the memory,
>>> @@ -181,7 +204,9 @@ static int xattr_foreach(struct xattr_iter *it,
>>>  		if (it->ofs >= EROFS_BLKSIZ) {
>>>  			BUG_ON(it->ofs > EROFS_BLKSIZ);
>>>  
>>> -			xattr_iter_fixup(it);
>>> +			err = xattr_iter_fixup(it);
>>> +			if (unlikely(err))
>>> +				goto out;
>>>  			it->ofs = 0;
>>>  		}
>>>  
>>> @@ -213,7 +238,10 @@ static int xattr_foreach(struct xattr_iter *it,
>>>  	while (processed < value_sz) {
>>>  		if (it->ofs >= EROFS_BLKSIZ) {
>>>  			BUG_ON(it->ofs > EROFS_BLKSIZ);
>>> -			xattr_iter_fixup(it);
>>> +
>>> +			err = xattr_iter_fixup(it);
>>> +			if (unlikely(err))
>>> +				goto out;
>>>  			it->ofs = 0;
>>>  		}
>>>  
>>> @@ -273,7 +301,7 @@ static void xattr_copyvalue(struct xattr_iter *_it,
>>>  	memcpy(it->buffer + processed, buf, len);
>>>  }
>>>  
>>> -static struct xattr_iter_handlers find_xattr_handlers = {
>>> +static const struct xattr_iter_handlers find_xattr_handlers = {
>>>  	.entry = xattr_entrymatch,
>>>  	.name = xattr_namematch,
>>>  	.alloc_buffer = xattr_checkbuffer,
>>> @@ -294,8 +322,11 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
>>>  		if ((ret = xattr_foreach(&it->it,
>>>  			&find_xattr_handlers, &remaining)) >= 0)
>>>  			break;
>>> +
>>> +		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
>>> +			break;
>>>  	}
>>> -	xattr_iter_end(&it->it, true);
>>> +	xattr_iter_end_final(&it->it);
>>>  
>>>  	return ret < 0 ? ret : it->buffer_size;
>>>  }
>>> @@ -318,9 +349,10 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>>>  			if (i)
>>>  				xattr_iter_end(&it->it, true);
>>>  
>>> -			it->it.page = erofs_get_meta_page_nofail(sb,
>>> -				blkaddr, false);
>>> -			BUG_ON(IS_ERR(it->it.page));
>>> +			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
>>> +			if (unlikely(IS_ERR(it->it.page)))
>>> +				return PTR_ERR(it->it.page);
>>> +
>>>  			it->it.kaddr = kmap_atomic(it->it.page);
>>>  			it->it.blkaddr = blkaddr;
>>>  		}
>>> @@ -328,9 +360,12 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
>>>  		if ((ret = xattr_foreach(&it->it,
>>>  			&find_xattr_handlers, NULL)) >= 0)
>>>  			break;
>>> +
>>> +		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
>>> +			break;
>>>  	}
>>>  	if (vi->xattr_shared_count)
>>> -		xattr_iter_end(&it->it, true);
>>> +		xattr_iter_end_final(&it->it);
>>>  
>>>  	return ret < 0 ? ret : it->buffer_size;
>>>  }
>>> @@ -355,7 +390,9 @@ int erofs_getxattr(struct inode *inode, int index,
>>>  	if (unlikely(name == NULL))
>>>  		return -EINVAL;
>>>  
>>> -	init_inode_xattrs(inode);
>>> +	ret = init_inode_xattrs(inode);
>>> +	if (unlikely(ret < 0))
>>> +		return ret;
>>>  
>>>  	it.index = index;
>>>  
>>> @@ -498,7 +535,7 @@ static int xattr_skipvalue(struct xattr_iter *_it,
>>>  	return 1;
>>>  }
>>>  
>>> -static struct xattr_iter_handlers list_xattr_handlers = {
>>> +static const struct xattr_iter_handlers list_xattr_handlers = {
>>>  	.entry = xattr_entrylist,
>>>  	.name = xattr_namelist,
>>>  	.alloc_buffer = xattr_skipvalue,
>>> @@ -520,7 +557,7 @@ static int inline_listxattr(struct listxattr_iter *it)
>>>  			&list_xattr_handlers, &remaining)) < 0)
>>>  			break;
>>>  	}
>>> -	xattr_iter_end(&it->it, true);
>>> +	xattr_iter_end_final(&it->it);
>>>  	return ret < 0 ? ret : it->buffer_ofs;
>>>  }
>>>  
>>> @@ -542,9 +579,10 @@ static int shared_listxattr(struct listxattr_iter *it)
>>>  			if (i)
>>>  				xattr_iter_end(&it->it, true);
>>>  
>>> -			it->it.page = erofs_get_meta_page_nofail(sb,
>>> -				blkaddr, false);
>>> -			BUG_ON(IS_ERR(it->it.page));
>>> +			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
>>> +			if (unlikely(IS_ERR(it->it.page)))
>>> +				return PTR_ERR(it->it.page);
>>> +
>>>  			it->it.kaddr = kmap_atomic(it->it.page);
>>>  			it->it.blkaddr = blkaddr;
>>>  		}
>>> @@ -554,7 +592,7 @@ static int shared_listxattr(struct listxattr_iter *it)
>>>  			break;
>>>  	}
>>>  	if (vi->xattr_shared_count)
>>> -		xattr_iter_end(&it->it, true);
>>> +		xattr_iter_end_final(&it->it);
>>>  
>>>  	return ret < 0 ? ret : it->buffer_ofs;
>>>  }
>>> @@ -565,7 +603,9 @@ ssize_t erofs_listxattr(struct dentry *dentry,
>>>  	int ret;
>>>  	struct listxattr_iter it;
>>>  
>>> -	init_inode_xattrs(d_inode(dentry));
>>> +	ret = init_inode_xattrs(d_inode(dentry));
>>> +	if (unlikely(ret < 0))
>>> +		return ret;
>>>  
>>>  	it.dentry = dentry;
>>>  	it.buffer = buffer;
>>>
>>
> 
> .
> 

[PREVIEW] [PATCH 4/4] staging: erofs: add error handling for xattr submodule

[Gao Xiang]

Hi Chao,

On 2018/8/10 14:08, Chao Yu wrote:
> Hi Xiang,
> 
> On 2018/8/10 11:47, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2018/8/10 11:40, Chao Yu wrote:
>>> On 2018/8/1 14:01, Gao Xiang wrote:
>>>> This patch enhances the missing error handling code for
>>>> xattr submodule, which improves the stability for the rare cases.
>>>>
>>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>>> ---
>>>>  drivers/staging/erofs/xattr.c | 114 ++++++++++++++++++++++++++++--------------
>>>>  1 file changed, 77 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
>>>> index 702434b..bd911bb 100644
>>>> --- a/drivers/staging/erofs/xattr.c
>>>> +++ b/drivers/staging/erofs/xattr.c
>>>> @@ -24,16 +24,25 @@ struct xattr_iter {
>>>>  
>>>>  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>>>>  {
>>>> -	/* only init_inode_xattrs use non-atomic once */
>>>> +	/* the only user of kunmap() is 'init_inode_xattrs' */
>>>>  	if (unlikely(!atomic))
>>>>  		kunmap(it->page);
>>>>  	else
>>>>  		kunmap_atomic(it->kaddr);
>>>> +
>>>>  	unlock_page(it->page);
>>>>  	put_page(it->page);
>>>>  }
>>>>  
>>>> -static void init_inode_xattrs(struct inode *inode)
>>>> +static inline void xattr_iter_end_final(struct xattr_iter *it)
>>>> +{
>>>> +	if (unlikely(it->page == NULL))
>>>> +		return;
>>>> +
>>>> +	xattr_iter_end(it, true);
>>>> +}
>>>> +
>>>> +static int init_inode_xattrs(struct inode *inode)
>>>>  {
>>>>  	struct xattr_iter it;
>>>>  	unsigned i;
>>>> @@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
>>>>  	bool atomic_map;
>>>>  
>>>>  	if (likely(inode_has_inited_xattr(inode)))
>>>> -		return;
>>>> +		return 0;
>>>>  
>>>>  	vi = EROFS_V(inode);
>>>>  	BUG_ON(!vi->xattr_isize);
>>>> @@ -55,7 +64,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>>  	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>>>>  
>>>>  	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
>>> Need to introduce and use erofs_get_inline_page()?
>>>
>>>> -	BUG_ON(IS_ERR(it.page));
>>>> +	if (unlikely(IS_ERR(it.page)))
>>>> +		return PTR_ERR(it.page);
>>>>  
>>>>  	/* read in shared xattr array (non-atomic, see kmalloc below) */
>>>>  	it.kaddr = kmap(it.page);
>>>> @@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
>>>>  	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>>>>  
>>>>  	vi->xattr_shared_count = ih->h_shared_count;
>>>> -	vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
>>>> -		vi->xattr_shared_count, sizeof(unsigned),
>>>> -		GFP_KERNEL | __GFP_NOFAIL);
>>>> +	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>>>> +						sizeof(unsigned), GFP_KERNEL);
>>>> +	if (unlikely(vi->xattr_shared_xattrs == NULL)) {
>>>> +		xattr_iter_end(&it, atomic_map);
>>>> +		return -ENOMEM;
>>>> +	}
>>>>  
>>>>  	/* let's skip ibody header */
>>>>  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
>>>> @@ -79,7 +92,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>>  
>>>>  			it.page = erofs_get_meta_page_nofail(sb,
>>>>  				++it.blkaddr, S_ISDIR(inode->i_mode));
>>> Need to use erofs_get_meta_page() instead?
>>>
>>> Thanks,
>> My consideration is for example,
>>
>> init_inode_xattrs itself can be seen as a part of inode initialization (other fs does it in
>> inode initialization, but erofs does it in the first xattr request), if it fail, the only way
>> is to retry again...but with more overhead to retry...
> I think we can easily return an error to user no matter we encounter -ENOMEM or
> -EIO, so erofs_get_meta_page() is enough here, thoughts?
> 

OK, it depends on users (kernel / programs). I think they will simply retry again in the outer loop,
which will cause more overhead therefore for inode metadata (not xattrs), I personally tend to retry inside.

That is just my personal opinion, will fix it.

Thanks,
Gao Xiang

> Thanks,
> 

[PREVIEW] [PATCH 4/4] staging: erofs: add error handling for xattr submodule

[Chao Yu]

Hi Xiang,

On 2018/8/10 14:15, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/8/10 14:08, Chao Yu wrote:
>> Hi Xiang,
>>
>> On 2018/8/10 11:47, Gao Xiang wrote:
>>> Hi Chao,
>>>
>>> On 2018/8/10 11:40, Chao Yu wrote:
>>>> On 2018/8/1 14:01, Gao Xiang wrote:
>>>>> This patch enhances the missing error handling code for
>>>>> xattr submodule, which improves the stability for the rare cases.
>>>>>
>>>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>>>> ---
>>>>>  drivers/staging/erofs/xattr.c | 114 ++++++++++++++++++++++++++++--------------
>>>>>  1 file changed, 77 insertions(+), 37 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
>>>>> index 702434b..bd911bb 100644
>>>>> --- a/drivers/staging/erofs/xattr.c
>>>>> +++ b/drivers/staging/erofs/xattr.c
>>>>> @@ -24,16 +24,25 @@ struct xattr_iter {
>>>>>  
>>>>>  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>>>>>  {
>>>>> -	/* only init_inode_xattrs use non-atomic once */
>>>>> +	/* the only user of kunmap() is 'init_inode_xattrs' */
>>>>>  	if (unlikely(!atomic))
>>>>>  		kunmap(it->page);
>>>>>  	else
>>>>>  		kunmap_atomic(it->kaddr);
>>>>> +
>>>>>  	unlock_page(it->page);
>>>>>  	put_page(it->page);
>>>>>  }
>>>>>  
>>>>> -static void init_inode_xattrs(struct inode *inode)
>>>>> +static inline void xattr_iter_end_final(struct xattr_iter *it)
>>>>> +{
>>>>> +	if (unlikely(it->page == NULL))
>>>>> +		return;
>>>>> +
>>>>> +	xattr_iter_end(it, true);
>>>>> +}
>>>>> +
>>>>> +static int init_inode_xattrs(struct inode *inode)
>>>>>  {
>>>>>  	struct xattr_iter it;
>>>>>  	unsigned i;
>>>>> @@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>  	bool atomic_map;
>>>>>  
>>>>>  	if (likely(inode_has_inited_xattr(inode)))
>>>>> -		return;
>>>>> +		return 0;
>>>>>  
>>>>>  	vi = EROFS_V(inode);
>>>>>  	BUG_ON(!vi->xattr_isize);
>>>>> @@ -55,7 +64,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>  	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>>>>>  
>>>>>  	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
>>>> Need to introduce and use erofs_get_inline_page()?
>>>>
>>>>> -	BUG_ON(IS_ERR(it.page));
>>>>> +	if (unlikely(IS_ERR(it.page)))
>>>>> +		return PTR_ERR(it.page);
>>>>>  
>>>>>  	/* read in shared xattr array (non-atomic, see kmalloc below) */
>>>>>  	it.kaddr = kmap(it.page);
>>>>> @@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>  	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>>>>>  
>>>>>  	vi->xattr_shared_count = ih->h_shared_count;
>>>>> -	vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
>>>>> -		vi->xattr_shared_count, sizeof(unsigned),
>>>>> -		GFP_KERNEL | __GFP_NOFAIL);
>>>>> +	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>>>>> +						sizeof(unsigned), GFP_KERNEL);
>>>>> +	if (unlikely(vi->xattr_shared_xattrs == NULL)) {
>>>>> +		xattr_iter_end(&it, atomic_map);
>>>>> +		return -ENOMEM;
>>>>> +	}
>>>>>  
>>>>>  	/* let's skip ibody header */
>>>>>  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
>>>>> @@ -79,7 +92,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>  
>>>>>  			it.page = erofs_get_meta_page_nofail(sb,
>>>>>  				++it.blkaddr, S_ISDIR(inode->i_mode));
>>>> Need to use erofs_get_meta_page() instead?
>>>>
>>>> Thanks,
>>> My consideration is for example,
>>>
>>> init_inode_xattrs itself can be seen as a part of inode initialization (other fs does it in
>>> inode initialization, but erofs does it in the first xattr request), if it fail, the only way
>>> is to retry again...but with more overhead to retry...
>> I think we can easily return an error to user no matter we encounter -ENOMEM or
>> -EIO, so erofs_get_meta_page() is enough here, thoughts?
>>
> 
> OK, it depends on users (kernel / programs). I think they will simply retry again in the outer loop,
> which will cause more overhead therefore for inode metadata (not xattrs), I personally tend to retry inside.

Yes, both are okay, as there is no such restriction in manual or other documents
saying syscall should success any time, so I think let's take easy to let user
to handle this condition.

And one more concern is, for extremely environment, like with very few memory,
retrying in 'nofail' function, make syscall looks like hanging in kernel side,
result in apps can not respond to user, that's not a good experience for user.

Also like f2fs, nofail version function is abused, I think it needs to be
limited and it's better to be rectified like other filesystem.

Thanks,

> 
> That is just my personal opinion, will fix it.
> 
> Thanks,
> Gao Xiang
> 
>> Thanks,
>>
> 
> .
> 

[PREVIEW] [PATCH 4/4] staging: erofs: add error handling for xattr submodule

[Gao Xiang]

On 2018/8/10 14:33, Chao Yu wrote:
> Hi Xiang,
> 
> On 2018/8/10 14:15, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2018/8/10 14:08, Chao Yu wrote:
>>> Hi Xiang,
>>>
>>> On 2018/8/10 11:47, Gao Xiang wrote:
>>>> Hi Chao,
>>>>
>>>> On 2018/8/10 11:40, Chao Yu wrote:
>>>>> On 2018/8/1 14:01, Gao Xiang wrote:
>>>>>> This patch enhances the missing error handling code for
>>>>>> xattr submodule, which improves the stability for the rare cases.
>>>>>>
>>>>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>>>>> ---
>>>>>>  drivers/staging/erofs/xattr.c | 114 ++++++++++++++++++++++++++++--------------
>>>>>>  1 file changed, 77 insertions(+), 37 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
>>>>>> index 702434b..bd911bb 100644
>>>>>> --- a/drivers/staging/erofs/xattr.c
>>>>>> +++ b/drivers/staging/erofs/xattr.c
>>>>>> @@ -24,16 +24,25 @@ struct xattr_iter {
>>>>>>  
>>>>>>  static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
>>>>>>  {
>>>>>> -	/* only init_inode_xattrs use non-atomic once */
>>>>>> +	/* the only user of kunmap() is 'init_inode_xattrs' */
>>>>>>  	if (unlikely(!atomic))
>>>>>>  		kunmap(it->page);
>>>>>>  	else
>>>>>>  		kunmap_atomic(it->kaddr);
>>>>>> +
>>>>>>  	unlock_page(it->page);
>>>>>>  	put_page(it->page);
>>>>>>  }
>>>>>>  
>>>>>> -static void init_inode_xattrs(struct inode *inode)
>>>>>> +static inline void xattr_iter_end_final(struct xattr_iter *it)
>>>>>> +{
>>>>>> +	if (unlikely(it->page == NULL))
>>>>>> +		return;
>>>>>> +
>>>>>> +	xattr_iter_end(it, true);
>>>>>> +}
>>>>>> +
>>>>>> +static int init_inode_xattrs(struct inode *inode)
>>>>>>  {
>>>>>>  	struct xattr_iter it;
>>>>>>  	unsigned i;
>>>>>> @@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>>  	bool atomic_map;
>>>>>>  
>>>>>>  	if (likely(inode_has_inited_xattr(inode)))
>>>>>> -		return;
>>>>>> +		return 0;
>>>>>>  
>>>>>>  	vi = EROFS_V(inode);
>>>>>>  	BUG_ON(!vi->xattr_isize);
>>>>>> @@ -55,7 +64,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>>  	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
>>>>>>  
>>>>>>  	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
>>>>> Need to introduce and use erofs_get_inline_page()?
>>>>>
>>>>>> -	BUG_ON(IS_ERR(it.page));
>>>>>> +	if (unlikely(IS_ERR(it.page)))
>>>>>> +		return PTR_ERR(it.page);
>>>>>>  
>>>>>>  	/* read in shared xattr array (non-atomic, see kmalloc below) */
>>>>>>  	it.kaddr = kmap(it.page);
>>>>>> @@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>>  	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>>>>>>  
>>>>>>  	vi->xattr_shared_count = ih->h_shared_count;
>>>>>> -	vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
>>>>>> -		vi->xattr_shared_count, sizeof(unsigned),
>>>>>> -		GFP_KERNEL | __GFP_NOFAIL);
>>>>>> +	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>>>>>> +						sizeof(unsigned), GFP_KERNEL);
>>>>>> +	if (unlikely(vi->xattr_shared_xattrs == NULL)) {
>>>>>> +		xattr_iter_end(&it, atomic_map);
>>>>>> +		return -ENOMEM;
>>>>>> +	}
>>>>>>  
>>>>>>  	/* let's skip ibody header */
>>>>>>  	it.ofs += sizeof(struct erofs_xattr_ibody_header);
>>>>>> @@ -79,7 +92,8 @@ static void init_inode_xattrs(struct inode *inode)
>>>>>>  
>>>>>>  			it.page = erofs_get_meta_page_nofail(sb,
>>>>>>  				++it.blkaddr, S_ISDIR(inode->i_mode));
>>>>> Need to use erofs_get_meta_page() instead?
>>>>>
>>>>> Thanks,
>>>> My consideration is for example,
>>>>
>>>> init_inode_xattrs itself can be seen as a part of inode initialization (other fs does it in
>>>> inode initialization, but erofs does it in the first xattr request), if it fail, the only way
>>>> is to retry again...but with more overhead to retry...
>>> I think we can easily return an error to user no matter we encounter -ENOMEM or
>>> -EIO, so erofs_get_meta_page() is enough here, thoughts?
>>>
>> OK, it depends on users (kernel / programs). I think they will simply retry again in the outer loop,
>> which will cause more overhead therefore for inode metadata (not xattrs), I personally tend to retry inside.
> Yes, both are okay, as there is no such restriction in manual or other documents
> saying syscall should success any time, so I think let's take easy to let user
> to handle this condition.
> 
> And one more concern is, for extremely environment, like with very few memory,
> retrying in 'nofail' function, make syscall looks like hanging in kernel side,
> result in apps can not respond to user, that's not a good experience for user.
> 
> Also like f2fs, nofail version function is abused, I think it needs to be
> limited and it's better to be rectified like other filesystem.
> 

Agreed. Thanks for your explainations. :)

Thanks,
Gao Xiang

> Thanks,
> 

[PREVIEW] [PATCH v3 3/3] staging: erofs: add error handling for xattr submodule

[Gao Xiang]

This patch enhances the missing error handling code for
xattr submodule, which improves the stability for the rare cases.

Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
change log v3:
 - avoid all nofail usage according to Chao's suggestion;
 - remove all 'unlikely' hints together with IS_ERR.

 drivers/staging/erofs/internal.h |   6 +-
 drivers/staging/erofs/xattr.c    | 120 ++++++++++++++++++++++++++-------------
 2 files changed, 83 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index ecb6c02..db8b734 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -479,10 +479,10 @@ struct erofs_map_blocks_iter {
 
 
 static inline struct page *
-erofs_get_inline_page_nofail(struct inode *inode,
-			     erofs_blk_t blkaddr)
+erofs_get_inline_page(struct inode *inode,
+		      erofs_blk_t blkaddr)
 {
-	return erofs_get_meta_page_nofail(inode->i_sb,
+	return erofs_get_meta_page(inode->i_sb,
 		blkaddr, S_ISDIR(inode->i_mode));
 }
 
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 2593c85..1b5815f 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -24,16 +24,25 @@ struct xattr_iter {
 
 static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
 {
-	/* only init_inode_xattrs use non-atomic once */
+	/* the only user of kunmap() is 'init_inode_xattrs' */
 	if (unlikely(!atomic))
 		kunmap(it->page);
 	else
 		kunmap_atomic(it->kaddr);
+
 	unlock_page(it->page);
 	put_page(it->page);
 }
 
-static void init_inode_xattrs(struct inode *inode)
+static inline void xattr_iter_end_final(struct xattr_iter *it)
+{
+	if (unlikely(it->page == NULL))
+		return;
+
+	xattr_iter_end(it, true);
+}
+
+static int init_inode_xattrs(struct inode *inode)
 {
 	struct xattr_iter it;
 	unsigned i;
@@ -44,7 +53,7 @@ static void init_inode_xattrs(struct inode *inode)
 	bool atomic_map;
 
 	if (likely(inode_has_inited_xattr(inode)))
-		return;
+		return 0;
 
 	vi = EROFS_V(inode);
 	BUG_ON(!vi->xattr_isize);
@@ -54,8 +63,9 @@ static void init_inode_xattrs(struct inode *inode)
 	it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
 	it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
 
-	it.page = erofs_get_inline_page_nofail(inode, it.blkaddr);
-	BUG_ON(IS_ERR(it.page));
+	it.page = erofs_get_inline_page(inode, it.blkaddr);
+	if (IS_ERR(it.page))
+		return PTR_ERR(it.page);
 
 	/* read in shared xattr array (non-atomic, see kmalloc below) */
 	it.kaddr = kmap(it.page);
@@ -64,9 +74,12 @@ static void init_inode_xattrs(struct inode *inode)
 	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
 
 	vi->xattr_shared_count = ih->h_shared_count;
-	vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
-		vi->xattr_shared_count, sizeof(unsigned),
-		GFP_KERNEL | __GFP_NOFAIL);
+	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
+						sizeof(unsigned), GFP_KERNEL);
+	if (unlikely(vi->xattr_shared_xattrs == NULL)) {
+		xattr_iter_end(&it, atomic_map);
+		return -ENOMEM;
+	}
 
 	/* let's skip ibody header */
 	it.ofs += sizeof(struct erofs_xattr_ibody_header);
@@ -77,9 +90,10 @@ static void init_inode_xattrs(struct inode *inode)
 			BUG_ON(it.ofs != EROFS_BLKSIZ);
 			xattr_iter_end(&it, atomic_map);
 
-			it.page = erofs_get_meta_page_nofail(sb,
+			it.page = erofs_get_meta_page(sb,
 				++it.blkaddr, S_ISDIR(inode->i_mode));
-			BUG_ON(IS_ERR(it.page));
+			if (IS_ERR(it.page))
+				return PTR_ERR(it.page);
 
 			it.kaddr = kmap_atomic(it.page);
 			atomic_map = true;
@@ -92,6 +106,7 @@ static void init_inode_xattrs(struct inode *inode)
 	xattr_iter_end(&it, atomic_map);
 
 	inode_set_inited_xattr(inode);
+	return 0;
 }
 
 struct xattr_iter_handlers {
@@ -101,19 +116,24 @@ struct xattr_iter_handlers {
 	void (*value)(struct xattr_iter *, unsigned, char *, unsigned);
 };
 
-static void xattr_iter_fixup(struct xattr_iter *it)
+static inline int xattr_iter_fixup(struct xattr_iter *it)
 {
-	if (unlikely(it->ofs >= EROFS_BLKSIZ)) {
-		xattr_iter_end(it, true);
+	if (likely(it->ofs < EROFS_BLKSIZ))
+		return 0;
 
-		it->blkaddr += erofs_blknr(it->ofs);
-		it->page = erofs_get_meta_page_nofail(it->sb,
-			it->blkaddr, false);
-		BUG_ON(IS_ERR(it->page));
+	xattr_iter_end(it, true);
 
-		it->kaddr = kmap_atomic(it->page);
-		it->ofs = erofs_blkoff(it->ofs);
+	it->blkaddr += erofs_blknr(it->ofs);
+
+	it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
+	if (IS_ERR(it->page)) {
+		it->page = NULL;
+		return PTR_ERR(it->page);
 	}
+
+	it->kaddr = kmap_atomic(it->page);
+	it->ofs = erofs_blkoff(it->ofs);
+	return 0;
 }
 
 static int inline_xattr_iter_begin(struct xattr_iter *it,
@@ -134,22 +154,25 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
 	it->blkaddr = erofs_blknr(iloc(sbi, vi->nid) + inline_xattr_ofs);
 	it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
 
-	it->page = erofs_get_inline_page_nofail(inode, it->blkaddr);
-	BUG_ON(IS_ERR(it->page));
-	it->kaddr = kmap_atomic(it->page);
+	it->page = erofs_get_inline_page(inode, it->blkaddr);
+	if (IS_ERR(it->page))
+		return PTR_ERR(it->page);
 
+	it->kaddr = kmap_atomic(it->page);
 	return vi->xattr_isize - xattr_header_sz;
 }
 
 static int xattr_foreach(struct xattr_iter *it,
-	struct xattr_iter_handlers *op, unsigned *tlimit)
+	const struct xattr_iter_handlers *op, unsigned *tlimit)
 {
 	struct erofs_xattr_entry entry;
 	unsigned value_sz, processed, slice;
 	int err;
 
 	/* 0. fixup blkaddr, ofs, ipage */
-	xattr_iter_fixup(it);
+	err = xattr_iter_fixup(it);
+	if (unlikely(err))
+		return err;
 
 	/*
 	 * 1. read xattr entry to the memory,
@@ -181,7 +204,9 @@ static int xattr_foreach(struct xattr_iter *it,
 		if (it->ofs >= EROFS_BLKSIZ) {
 			BUG_ON(it->ofs > EROFS_BLKSIZ);
 
-			xattr_iter_fixup(it);
+			err = xattr_iter_fixup(it);
+			if (unlikely(err))
+				goto out;
 			it->ofs = 0;
 		}
 
@@ -213,7 +238,10 @@ static int xattr_foreach(struct xattr_iter *it,
 	while (processed < value_sz) {
 		if (it->ofs >= EROFS_BLKSIZ) {
 			BUG_ON(it->ofs > EROFS_BLKSIZ);
-			xattr_iter_fixup(it);
+
+			err = xattr_iter_fixup(it);
+			if (unlikely(err))
+				goto out;
 			it->ofs = 0;
 		}
 
@@ -273,7 +301,7 @@ static void xattr_copyvalue(struct xattr_iter *_it,
 	memcpy(it->buffer + processed, buf, len);
 }
 
-static struct xattr_iter_handlers find_xattr_handlers = {
+static const struct xattr_iter_handlers find_xattr_handlers = {
 	.entry = xattr_entrymatch,
 	.name = xattr_namematch,
 	.alloc_buffer = xattr_checkbuffer,
@@ -294,8 +322,11 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
 		ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining);
 		if (ret >= 0)
 			break;
+
+		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
+			break;
 	}
-	xattr_iter_end(&it->it, true);
+	xattr_iter_end_final(&it->it);
 
 	return ret < 0 ? ret : it->buffer_size;
 }
@@ -318,9 +349,10 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page_nofail(sb,
-				blkaddr, false);
-			BUG_ON(IS_ERR(it->it.page));
+			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
+			if (IS_ERR(it->it.page))
+				return PTR_ERR(it->it.page);
+
 			it->it.kaddr = kmap_atomic(it->it.page);
 			it->it.blkaddr = blkaddr;
 		}
@@ -328,9 +360,12 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
 		ret = xattr_foreach(&it->it, &find_xattr_handlers, NULL);
 		if (ret >= 0)
 			break;
+
+		if (unlikely(ret != -ENOATTR))	/* -ENOMEM, -EIO, etc. */
+			break;
 	}
 	if (vi->xattr_shared_count)
-		xattr_iter_end(&it->it, true);
+		xattr_iter_end_final(&it->it);
 
 	return ret < 0 ? ret : it->buffer_size;
 }
@@ -355,7 +390,9 @@ int erofs_getxattr(struct inode *inode, int index,
 	if (unlikely(name == NULL))
 		return -EINVAL;
 
-	init_inode_xattrs(inode);
+	ret = init_inode_xattrs(inode);
+	if (unlikely(ret < 0))
+		return ret;
 
 	it.index = index;
 
@@ -498,7 +535,7 @@ static int xattr_skipvalue(struct xattr_iter *_it,
 	return 1;
 }
 
-static struct xattr_iter_handlers list_xattr_handlers = {
+static const struct xattr_iter_handlers list_xattr_handlers = {
 	.entry = xattr_entrylist,
 	.name = xattr_namelist,
 	.alloc_buffer = xattr_skipvalue,
@@ -520,7 +557,7 @@ static int inline_listxattr(struct listxattr_iter *it)
 		if (ret < 0)
 			break;
 	}
-	xattr_iter_end(&it->it, true);
+	xattr_iter_end_final(&it->it);
 	return ret < 0 ? ret : it->buffer_ofs;
 }
 
@@ -542,9 +579,10 @@ static int shared_listxattr(struct listxattr_iter *it)
 			if (i)
 				xattr_iter_end(&it->it, true);
 
-			it->it.page = erofs_get_meta_page_nofail(sb,
-				blkaddr, false);
-			BUG_ON(IS_ERR(it->it.page));
+			it->it.page = erofs_get_meta_page(sb, blkaddr, false);
+			if (IS_ERR(it->it.page))
+				return PTR_ERR(it->it.page);
+
 			it->it.kaddr = kmap_atomic(it->it.page);
 			it->it.blkaddr = blkaddr;
 		}
@@ -554,7 +592,7 @@ static int shared_listxattr(struct listxattr_iter *it)
 			break;
 	}
 	if (vi->xattr_shared_count)
-		xattr_iter_end(&it->it, true);
+		xattr_iter_end_final(&it->it);
 
 	return ret < 0 ? ret : it->buffer_ofs;
 }
@@ -565,7 +603,9 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 	int ret;
 	struct listxattr_iter it;
 
-	init_inode_xattrs(d_inode(dentry));
+	ret = init_inode_xattrs(d_inode(dentry));
+	if (unlikely(ret < 0))
+		return ret;
 
 	it.dentry = dentry;
 	it.buffer = buffer;
-- 
1.9.1

[PREVIEW] [PATCH v3 3/3] staging: erofs: add error handling for xattr submodule

[Chao Yu]

On 2018/8/11 0:50, Gao Xiang wrote:
> This patch enhances the missing error handling code for
> xattr submodule, which improves the stability for the rare cases.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>

Reviewed-by: Chao Yu <yuchao0 at huawei.com>

Thanks,

[PREVIEW] [PATCH 1/4] staging: erofs: remove the unneeded marco in xattr submodule

[Chao Yu]

On 2018/8/1 14:01, Gao Xiang wrote:
> There is no need to '#if CONFIG_EROFS_FS_XATTR' in xattr.c,
> let's remove it.
> 
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>

Reviewed-by: Chao Yu <yuchao0 at huawei.com>

Thanks,