* [bug report] staging: erofs: introduce erofs_grab_bio
@ 2019-08-28 10:55 Dan Carpenter
2019-08-28 11:02 ` Gao Xiang
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-08-28 10:55 UTC (permalink / raw)
To: xiang; +Cc: linux-erofs
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.
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] staging: erofs: introduce erofs_grab_bio
2019-08-28 10:55 [bug report] staging: erofs: introduce erofs_grab_bio Dan Carpenter
@ 2019-08-28 11:02 ` Gao Xiang
2019-08-28 11:36 ` Dan Carpenter
2019-08-28 11:39 ` Gao Xiang
0 siblings, 2 replies; 7+ messages in thread
From: Gao Xiang @ 2019-08-28 11:02 UTC (permalink / raw)
To: Dan Carpenter; +Cc: miaoxie, xiang, linux-erofs
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] staging: erofs: introduce erofs_grab_bio
2019-08-28 11:02 ` Gao Xiang
@ 2019-08-28 11:36 ` Dan Carpenter
2019-08-28 11:40 ` Gao Xiang
2019-08-28 11:39 ` Gao Xiang
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-08-28 11:36 UTC (permalink / raw)
To: Gao Xiang; +Cc: miaoxie, xiang, linux-erofs
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?
>
Ah. Yes. You're right.
regards,
dan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] staging: erofs: introduce erofs_grab_bio
2019-08-28 11:02 ` Gao Xiang
2019-08-28 11:36 ` Dan Carpenter
@ 2019-08-28 11:39 ` Gao Xiang
2019-08-28 12:11 ` Dan Carpenter
1 sibling, 1 reply; 7+ messages in thread
From: Gao Xiang @ 2019-08-28 11:39 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-erofs, xiang, miaoxie
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] staging: erofs: introduce erofs_grab_bio
2019-08-28 11:36 ` Dan Carpenter
@ 2019-08-28 11:40 ` Gao Xiang
0 siblings, 0 replies; 7+ messages in thread
From: Gao Xiang @ 2019-08-28 11:40 UTC (permalink / raw)
To: Dan Carpenter; +Cc: miaoxie, xiang, linux-erofs
On Wed, Aug 28, 2019 at 02:36:13PM +0300, Dan Carpenter wrote:
> 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?
> >
>
> Ah. Yes. You're right.
That is fine :) Thanks,
Cheers,
Gao Xiang
>
> regards,
> dan
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] staging: erofs: introduce erofs_grab_bio
2019-08-28 11:39 ` Gao Xiang
@ 2019-08-28 12:11 ` Dan Carpenter
2019-08-28 12:22 ` Gao Xiang
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-08-28 12:11 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-erofs, xiang, miaoxie
On Wed, Aug 28, 2019 at 07:39:29PM +0800, Gao Xiang wrote:
> (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...)
Of course I thought it *was* a bug...
I've sent probably 1800 of these emails. It's a script but I look over
the email before sending. Maybe when people start using the Link: tag
I will be able to make these show up as reply to an email.
Normally, I sent them out in a much more timely sort of way but all the
erofs warnings show up as new with the move out of staging so I have
been re-reviewing the warnings.
So last August when this code was new, I must have seen the warning but
read the code correctly. I checked before I sent this email to make
sure we hadn't discusssed it before.
But this time I got confused by the DBG_BUGON(). I decided to treat it
as a no-op because it can be configured to do nothing if you have
CONFIG_EROFS_FS_DEBUG disabled. Plus it has "DBG" in the name so it
felt like debug code. But I ended up focussing on it instead of seeing
the "(nofail ? __GFP_NOFAIL : 0)" bit. The DBG_BUGON() is unreachable
and misleading nonsense fluff. :(
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] staging: erofs: introduce erofs_grab_bio
2019-08-28 12:11 ` Dan Carpenter
@ 2019-08-28 12:22 ` Gao Xiang
0 siblings, 0 replies; 7+ messages in thread
From: Gao Xiang @ 2019-08-28 12:22 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-erofs, xiang, miaoxie
Hi Dan,
On Wed, Aug 28, 2019 at 03:11:44PM +0300, Dan Carpenter wrote:
> On Wed, Aug 28, 2019 at 07:39:29PM +0800, Gao Xiang wrote:
> > (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...)
>
> Of course I thought it *was* a bug...
>
> I've sent probably 1800 of these emails. It's a script but I look over
> the email before sending. Maybe when people start using the Link: tag
> I will be able to make these show up as reply to an email.
Thanks for your effort to communities [thumb]
>
> Normally, I sent them out in a much more timely sort of way but all the
> erofs warnings show up as new with the move out of staging so I have
> been re-reviewing the warnings.
>
> So last August when this code was new, I must have seen the warning but
> read the code correctly. I checked before I sent this email to make
> sure we hadn't discusssed it before.
>
> But this time I got confused by the DBG_BUGON(). I decided to treat it
> as a no-op because it can be configured to do nothing if you have
> CONFIG_EROFS_FS_DEBUG disabled. Plus it has "DBG" in the name so it
> felt like debug code. But I ended up focussing on it instead of seeing
> the "(nofail ? __GFP_NOFAIL : 0)" bit. The DBG_BUGON() is unreachable
> and misleading nonsense fluff. :(
I fully understand that :) That is fine.
In a word, thanks for reporting :)
Thanks,
Gao Xiang
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-28 12:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-28 10:55 [bug report] staging: erofs: introduce erofs_grab_bio Dan Carpenter
2019-08-28 11:02 ` Gao Xiang
2019-08-28 11:36 ` Dan Carpenter
2019-08-28 11:40 ` Gao Xiang
2019-08-28 11:39 ` Gao Xiang
2019-08-28 12:11 ` Dan Carpenter
2019-08-28 12:22 ` Gao Xiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).