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

[Gao Xiang <gaoxiang25@huawei.com>]

On Wed, Aug 28, 2019 at 07:02:49PM +0800, Gao Xiang wrote:
> 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?

Add a word about this, since EROFS submit chain has been built firmly here, we have to
introduce another full complete bailout path to error out all these physical pages and
all corresponding logical pages if we don't use __GFP_NOFAIL here.

Since bio structure allocation isn't costly here at all (many many on-market commerical
products can prove that), I tend to leave it as-is for now...

(p.s. It makes me little confused these subject prefixes are "[bug report]", if they are
really bugs, that is fine... If it be something unconfirmed (need our confirmation..,),
could you kindly change the prefix into some other representations...? I will still look
into all of them at least... and that makes me feel a bit better and easy.... thanks...)

Thanks,
Gao Xiang

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