Re: [bug report] staging: erofs: introduce erofs_grab_bio

From: Gao Xiang <gaoxiang25@huawei.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: miaoxie@huawei.com, xiang@kernel.org, linux-erofs@lists.ozlabs.org
Subject: Re: [bug report] staging: erofs: introduce erofs_grab_bio
Date: Wed, 28 Aug 2019 19:02:49 +0800	[thread overview]
Message-ID: <20190828110249.GA56298@architecture4> (raw)
In-Reply-To: <20190828105541.GA21320@mwanda>

Hi Dan,

On Wed, Aug 28, 2019 at 01:55:41PM +0300, Dan Carpenter wrote:
> Hello Gao Xiang,
> 
> The patch 8be31270362b: "staging: erofs: introduce erofs_grab_bio"
> from Aug 21, 2018, leads to the following static checker warning:
> 
> 	fs/erofs/zdata.c:1272 z_erofs_vle_submit_all()
> 	error: 'bio' dereferencing possible ERR_PTR()
> 
> fs/erofs/zdata.c
>   1259                  if (bio && force_submit) {
>   1260  submit_bio_retry:
>   1261                          __submit_bio(bio, REQ_OP_READ, 0);
>   1262                          bio = NULL;
>   1263                  }
>   1264  
>   1265                  if (!bio) {
>   1266                          bio = erofs_grab_bio(sb, first_index + i,
>   1267                                               BIO_MAX_PAGES, bi_private,
>   1268                                               z_erofs_vle_read_endio, true);
> 
> This assumes erofs_grab_bio() can't fail.  It returns ERR_PTR(-ENOMEM)
> on failure.

I think there is no problem at all as well.
The last argument of erofs_grab_bio is nofail, and here is "true".

415 static inline struct bio *erofs_grab_bio(struct super_block *sb,
416                                          erofs_blk_t blkaddr,
417                                          unsigned int nr_pages,
418                                          void *bi_private, bio_end_io_t endio,
419                                          bool nofail)
420 {
421         const gfp_t gfp = GFP_NOIO;
422         struct bio *bio;
423
424         do {
425                 if (nr_pages == 1) {
426                         bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
427                         if (unlikely(!bio)) {
428                                 DBG_BUGON(nofail);
429                                 return ERR_PTR(-ENOMEM);
430                         }
431                         break;
432                 }
433                 bio = bio_alloc(gfp, nr_pages);
434                 nr_pages /= 2;
435         } while (unlikely(!bio));
436
437         bio->bi_end_io = endio;
438         bio_set_dev(bio, sb->s_bdev);
439         bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK;
440         bio->bi_private = bi_private;
441         return bio;
442 }

You can see __GFP_NOFAIL is set, Am I missing something?

Thanks,
Gao Xiang

> 
>   1269                          ++nr_bios;
>   1270                  }
>   1271  
>   1272                  err = bio_add_page(bio, page, PAGE_SIZE, 0);
>   1273                  if (err < PAGE_SIZE)
>   1274                          goto submit_bio_retry;
>   1275  
>   1276                  force_submit = false;
>   1277                  last_index = first_index + i;
>   1278  skippage:
>   1279                  if (++i < clusterpages)
>   1280                          goto repeat;
> 
> regards,
> dan carpenter