On Thu, Oct 21, 2010 at 07:41:54PM +0200, Boaz Harrosh wrote:
> > +#ifdef PDEBUG
> > + trace_printk("%s: io submitted io_end %p\n",
> > + io->io_end->inode->i_sb->s_id, io->io_end);
> > +#endif
> > + submit_bio(io->io_op, io->io_bio);
> > + ASSERT(!bio_flagged(bio, BIO_EOPNOTSUPP));
> > + bio_put(io->io_bio);
>
> The extra get/put is only done for the duration of the ASSERT above, right?
> I'd put a comment. And why don't you just call the _get here just before
> submit_bio instead of down at io_submit_init.
The bio layer isn't well documented, so a lot of this was written by
cargo cult programming. In this case, I was following what XFS did
when I originally wrote it. It's only in the past week when I've been
diving deep into the source and was able to conclude that yes, it's
only needed for the ASSERT.
One of the frustrating things about the bio layer is that all of the
different file systems use very different patterns about how they
access the bio layer --- which is perhaps caused by the lack of
documentation. But I finally did notice that the JFS code dodn't do a
bio_get() after calling bio_alloc(), and it was only when I did a lot
of source code tracing that I realized that it strictly speaking the
ASSERT isn't really even all that necessary, since we're not calling
using a discard or barrier op, which is the only thing that could
possibly set the EOPNOTSUPP flag anyway....
> > + do {
> > + bio = bio_alloc(GFP_NOIO, nvecs);
> > + nvecs >>= 1;
> > + } while (bio == NULL);
>
> This is surly bad. bio_alloc must be allowed to fail
> (Specially with GFP_NOIO). You should only loop down to
> 1 and then prepare to return -ENOMEM from this function
> and handle it properly in callers. (Or schedule and wait
> like below)
Copied exactly from the XFS code. I was under the (possibly mistaken)
assumption that the XFS code was a good place to copy from, since
everyone sings the praises of XFS.....
I do agree that bio_alloc() should be allowed to fail, although life
gets really tough deep in the writeback stack to throw a failure all
the way back out. This was on my todo list to fix, after I got things
basically working.
- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
