On 2010-10-21 19:41, Boaz Harrosh wrote:
quoted text
>> +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:

quoted text
>> +	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()).

quoted text
>> +	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

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/