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
> ---
> 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;
> +
