request_alignment vs file size, how to fix crash?

request_alignment vs file size, how to fix crash?

[Vladimir Sementsov-Ogievskiy]

Hi!

I found a crash, which may be simply triggered for images unaligned to request_alignment:

# ./qemu-io --image-opts -c 'write 0 512' driver=blkdebug,align=4096,image.driver=null-co,image.size=512
qemu-io: block/io.c:1505: bdrv_aligned_pwritev: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
Aborted (core dumped)

The problem is obvious: 512 is aligned to 4096 and becomes larger than file size.

I faced it after rebasing our downstream branches to newer Rhel versions. Seems that after some updates of alignment detection in file-posix.c, it started to detect 4096 alignment in our build environment, and iotest 152 started to crash (as it operates on file of 512 bytes).

My question is:

What is wrong? Should we restrict images to be aligned to request_alignment, or allow unaligned operations at EOF, if file is unaligned itself?

-- 
Best regards,
Vladimir

Re: request_alignment vs file size, how to fix crash?

[Vladimir Sementsov-Ogievskiy]

29.01.2020 21:01, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
> 
> I found a crash, which may be simply triggered for images unaligned to request_alignment:
> 
> # ./qemu-io --image-opts -c 'write 0 512' driver=blkdebug,align=4096,image.driver=null-co,image.size=512
> qemu-io: block/io.c:1505: bdrv_aligned_pwritev: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
> Aborted (core dumped)
> 
> The problem is obvious: 512 is aligned to 4096 and becomes larger than file size.
> 
> I faced it after rebasing our downstream branches to newer Rhel versions. Seems that after some updates of alignment detection in file-posix.c, it started to detect 4096 alignment in our build environment, and iotest 152 started to crash (as it operates on file of 512 bytes).
> 
> My question is:
> 
> What is wrong? Should we restrict images to be aligned to request_alignment, or allow unaligned operations at EOF, if file is unaligned itself?
> 


The problem started with commit

commit a6b257a08e3d72219f03e461a52152672fec0612
Author: Nir Soffer <nirsof@gmail.com>
Date:   Tue Aug 13 21:21:03 2019 +0300

     file-posix: Handle undetectable alignment


It sets request_alignment to 4k, if probing of align=1 succeeded.. I think it's wrong logic. It leads to crashes for images unaligned to 4k.

If we force alignment to be 4k, we at least should check that file size is aligned to 4k. Otherwise our assumption is definitely wrong.

And still, I doubt that it's correct to force alignment to 4k, for devices which doesn't request any alignment..


-- 
Best regards,
Vladimir

Re: request_alignment vs file size, how to fix crash?

[Kevin Wolf]

Am 30.01.2020 um 11:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2020 21:01, Vladimir Sementsov-Ogievskiy wrote:
> > Hi!
> > 
> > I found a crash, which may be simply triggered for images unaligned to request_alignment:
> > 
> > # ./qemu-io --image-opts -c 'write 0 512' driver=blkdebug,align=4096,image.driver=null-co,image.size=512
> > qemu-io: block/io.c:1505: bdrv_aligned_pwritev: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
> > Aborted (core dumped)
> > 
> > The problem is obvious: 512 is aligned to 4096 and becomes larger than file size.
> > 
> > I faced it after rebasing our downstream branches to newer Rhel versions. Seems that after some updates of alignment detection in file-posix.c, it started to detect 4096 alignment in our build environment, and iotest 152 started to crash (as it operates on file of 512 bytes).
> > 
> > My question is:
> > 
> > What is wrong? Should we restrict images to be aligned to request_alignment, or allow unaligned operations at EOF, if file is unaligned itself?
> > 
> 
> 
> The problem started with commit
> 
> commit a6b257a08e3d72219f03e461a52152672fec0612
> Author: Nir Soffer <nirsof@gmail.com>
> Date:   Tue Aug 13 21:21:03 2019 +0300
> 
>     file-posix: Handle undetectable alignment
> 
> 
> It sets request_alignment to 4k, if probing of align=1 succeeded.. I think it's wrong logic. It leads to crashes for images unaligned to 4k.
> 
> If we force alignment to be 4k, we at least should check that file size is aligned to 4k. Otherwise our assumption is definitely wrong.
> 
> And still, I doubt that it's correct to force alignment to 4k, for devices which doesn't request any alignment..

What backend is this? O_DIRECT with byte alignment sounds wrong, so I
wonder if your storage really can do this or whether we just failed to
detect the actual alignment.

I guess we could change the default to pick the largest size so that the
image size is still a multiple of it. But if the image size isn't even
aligned to 512 bytes, I think refusing to open the image with O_DIRECT
feels more correct (I would be okay with doing the same with > 512 byte
images, too, if the image size isn't a multiple of the alignment).

Kevin

Re: request_alignment vs file size, how to fix crash?

[Vladimir Sementsov-Ogievskiy]

30.01.2020 14:11, Kevin Wolf wrote:
> Am 30.01.2020 um 11:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.01.2020 21:01, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi!
>>>
>>> I found a crash, which may be simply triggered for images unaligned to request_alignment:
>>>
>>> # ./qemu-io --image-opts -c 'write 0 512' driver=blkdebug,align=4096,image.driver=null-co,image.size=512
>>> qemu-io: block/io.c:1505: bdrv_aligned_pwritev: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
>>> Aborted (core dumped)
>>>
>>> The problem is obvious: 512 is aligned to 4096 and becomes larger than file size.
>>>
>>> I faced it after rebasing our downstream branches to newer Rhel versions. Seems that after some updates of alignment detection in file-posix.c, it started to detect 4096 alignment in our build environment, and iotest 152 started to crash (as it operates on file of 512 bytes).
>>>
>>> My question is:
>>>
>>> What is wrong? Should we restrict images to be aligned to request_alignment, or allow unaligned operations at EOF, if file is unaligned itself?
>>>
>>
>>
>> The problem started with commit
>>
>> commit a6b257a08e3d72219f03e461a52152672fec0612
>> Author: Nir Soffer <nirsof@gmail.com>
>> Date:   Tue Aug 13 21:21:03 2019 +0300
>>
>>      file-posix: Handle undetectable alignment
>>
>>
>> It sets request_alignment to 4k, if probing of align=1 succeeded.. I think it's wrong logic. It leads to crashes for images unaligned to 4k.
>>
>> If we force alignment to be 4k, we at least should check that file size is aligned to 4k. Otherwise our assumption is definitely wrong.
>>
>> And still, I doubt that it's correct to force alignment to 4k, for devices which doesn't request any alignment..
> 
> What backend is this? O_DIRECT with byte alignment sounds wrong, so I
> wonder if your storage really can do this or whether we just failed to
> detect the actual alignment.

The problem was disabled odirect in virtuozzo container which lead to byte alignment. So, yes, it's on our part.

> 
> I guess we could change the default to pick the largest size so that the
> image size is still a multiple of it. But if the image size isn't even
> aligned to 512 bytes, I think refusing to open the image with O_DIRECT
> feels more correct (I would be okay with doing the same with > 512 byte
> images, too, if the image size isn't a multiple of the alignment).
> 

OK, I'll think about a patch for file-posix.c, and may be blkdebug too.

Also, we need to check it somewhere in generic layer too, to fail earlier than assertion above.


-- 
Best regards,
Vladimir

Re: request_alignment vs file size, how to fix crash?

[Kevin Wolf]

Am 30.01.2020 um 12:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 30.01.2020 14:11, Kevin Wolf wrote:
> > Am 30.01.2020 um 11:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 29.01.2020 21:01, Vladimir Sementsov-Ogievskiy wrote:
> > > > Hi!
> > > > 
> > > > I found a crash, which may be simply triggered for images unaligned to request_alignment:
> > > > 
> > > > # ./qemu-io --image-opts -c 'write 0 512' driver=blkdebug,align=4096,image.driver=null-co,image.size=512
> > > > qemu-io: block/io.c:1505: bdrv_aligned_pwritev: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
> > > > Aborted (core dumped)
> > > > 
> > > > The problem is obvious: 512 is aligned to 4096 and becomes larger than file size.
> > > > 
> > > > I faced it after rebasing our downstream branches to newer Rhel versions. Seems that after some updates of alignment detection in file-posix.c, it started to detect 4096 alignment in our build environment, and iotest 152 started to crash (as it operates on file of 512 bytes).
> > > > 
> > > > My question is:
> > > > 
> > > > What is wrong? Should we restrict images to be aligned to request_alignment, or allow unaligned operations at EOF, if file is unaligned itself?
> > > > 
> > > 
> > > 
> > > The problem started with commit
> > > 
> > > commit a6b257a08e3d72219f03e461a52152672fec0612
> > > Author: Nir Soffer <nirsof@gmail.com>
> > > Date:   Tue Aug 13 21:21:03 2019 +0300
> > > 
> > >      file-posix: Handle undetectable alignment
> > > 
> > > 
> > > It sets request_alignment to 4k, if probing of align=1 succeeded.. I think it's wrong logic. It leads to crashes for images unaligned to 4k.
> > > 
> > > If we force alignment to be 4k, we at least should check that file size is aligned to 4k. Otherwise our assumption is definitely wrong.
> > > 
> > > And still, I doubt that it's correct to force alignment to 4k, for devices which doesn't request any alignment..
> > 
> > What backend is this? O_DIRECT with byte alignment sounds wrong, so I
> > wonder if your storage really can do this or whether we just failed to
> > detect the actual alignment.
> 
> The problem was disabled odirect in virtuozzo container which lead to byte alignment. So, yes, it's on our part.

Oh, I see, so to QEMU it looked like it would do O_DIRECT and probing
was done, but what was actually opened was non-direct. Not sure if we
could possibly distinguish a situation like this from one where O_DIRECT
succeeds with byte alignment because the block was unallocated, but
would require larger alignment later.

> > I guess we could change the default to pick the largest size so that the
> > image size is still a multiple of it. But if the image size isn't even
> > aligned to 512 bytes, I think refusing to open the image with O_DIRECT
> > feels more correct (I would be okay with doing the same with > 512 byte
> > images, too, if the image size isn't a multiple of the alignment).
> > 
> 
> OK, I'll think about a patch for file-posix.c, and may be blkdebug too.
> 
> Also, we need to check it somewhere in generic layer too, to fail earlier than assertion above.

Yes, I agree, it should be checked while opening the image.

Kevin