From: Shivaram U. <shi...@qu...> - 2013-07-02 20:23:34
|
On Wed, Jul 3, 2013 at 1:25 AM, Shivaram Upadhyayula < shi...@qu...> wrote: > ... (removed) > > while (tio_index < tio->pg_cnt) { > .... (removed) > In the above loop, we add the 512 bytes to the bio in the first > iteration itself. In the second iteration of the above loops bytes == size > == 0 and so on till 256 and in the second iteration we call bio_add_page > with size 0. > > In the file-io case, as you mention its more an optimization. We end up > calling vfs_write() with zero count. > > Thinking of a simpler check, Wouldn't replacing while/for (tio_index < tio->pg_cnt) with while (size) {} work ? Regards, Shivaram > Regards, > Shivaram > > > > On Wed, Jul 3, 2013 at 1:06 AM, Arne Redlich <arn...@go...>wrote: > >> Shivaram, >> >> [sorry for sending it again - forgot to CC iet-devel last time around] >> >> 2013/6/30 Shivaram Upadhyayula <shi...@qu...>: >> > There are many issues with the current implementation >> > 1. target_tio has an extra tio_get() which prevents it from getting >> freed >> > 2. length and media_length calculations need to be 64 bit >> > 3. MAX_IO_SIZE is actually set to 1GB rather than 1MB >> > 4. If the length is greater than 1MB say 1MB + 512 then in the last >> > iteration we need to write 512 bytes but then tio pg_cnt is greater >> than 1, >> > so we end up with a bio_add_page() with size 0 >> > >> > Patch attached >> >> Thanks for your patch. I took the liberty of splitting it, and for now >> I only applied the fixes for (1) - (3). Please verify I didn't break >> anything inadvertently. >> >> The fix for (4) (reproduced below for those interested) needs to go >> into a different patch as it's not directly related to WRITE SAME. >> Also, I'm at the moment not fully clear whether it's an actual bug - >> did you run into issues / can you point out the problem for blockio? >> The fileio change does not look like a bug but rather like an >> optimization, right? >> >> Thanks, >> Arne >> >> diff --git a/kernel/block-io.c b/kernel/block-io.c >> index dffc4d0..7c9ea21 100644 >> --- a/kernel/block-io.c >> +++ b/kernel/block-io.c >> @@ -79,7 +79,7 @@ blockio_make_request(struct iet_volume *volume, >> struct tio *tio, int rw) >> init_completion(&tio_work->tio_complete); >> >> /* Main processing loop, allocate and fill all bios */ >> - while (tio_index < tio->pg_cnt) { >> + while (size && tio_index < tio->pg_cnt) { >> bio = bio_alloc(GFP_KERNEL, min(max_pages, >> BIO_MAX_PAGES)); >> if (!bio) { >> err = -ENOMEM; >> @@ -100,7 +100,7 @@ blockio_make_request(struct iet_volume *volume, >> struct tio *tio, int rw) >> atomic_inc(&tio_work->bios_remaining); >> >> /* Loop for filling bio */ >> - while (tio_index < tio->pg_cnt) { >> + while (size && tio_index < tio->pg_cnt) { >> unsigned int bytes = PAGE_SIZE; >> >> if (bytes > size) >> diff --git a/kernel/file-io.c b/kernel/file-io.c >> index 21a986a..8c4a7f7 100644 >> --- a/kernel/file-io.c >> +++ b/kernel/file-io.c >> @@ -34,7 +34,7 @@ static int fileio_make_request(struct iet_volume >> *lu, struct tio *tio, int rw) >> size = tio->size; >> ppos = tio->offset; >> >> - for (i = 0; i < tio->pg_cnt; i++) { >> + for (i = 0; i < tio->pg_cnt && size; i++) { >> page = tio->pvec[i]; >> assert(page); >> buf = page_address(page); >> > > > > -- > QUADStor Open Source Storage Virtualization : Thin Provisioning, Data > Deduplication, VAAI, High Availability > http://www.quadstor.com > -- QUADStor Open Source Storage Virtualization : Thin Provisioning, Data Deduplication, VAAI, High Availability http://www.quadstor.com |