On 2010-10-21 19:41, Boaz Harrosh wrote:
>> +void ext4_io_submit(struct ext4_io_submit *io)
>> +{
>> + if (!io->io_bio)
>> + return;
>> +#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.
Ted is using it to signal the reference from io:
>> + io->io_bio = 0;
which makes sense (even if you don't need it, as long as you don't deref
bio after calling submit_bio()).
>> + 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)
Since __GFP_WAIT is set, it'll never return NULL. And as long as you
don't allocate more than 1 before doing you submit_bio(), it should be
OK in this case.
--
Jens Axboe
--
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
