| Message ID | 20170804151013.13057-4-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/04/2017 10:10 AM, Denis V. Lunev wrote: > This would be actually strange and error prone. If truncate() nowadays > will fail, there is something fatally wrong. Let's check for that during > the actual work. > > The only fallback case is when the file is not zero initialized. In this > case we should switch to preallocation via fallocate(). I got confused by the commit message. Here's my attempt an an alternative, to see if I'm understanding the point of this patch: The code was trying to truncate a file to its current length, as an optimization to help decide whether an alternative prealloc mode was useful. But it forgot to check whether bdrv_getlength() succeeded, and dealing with that failure just complicates what was supposed to be a no-op probe for optimizing later operation. We will still properly fail later when an actual truncation attempt is made and the device can't support it. So when deciding how to set prealloc_mode while opening the device, all we really need is to check just the one condition that matters - knowing whether the device is zero initialized. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Markus Armbruster <armbru@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > --- > block/parallels.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) The code change makes sense at first glance, but I'm a bit reluctant to give R-b, since the commit message threw me off and I'm not familiar with the parallels code in the first place. > > diff --git a/block/parallels.c b/block/parallels.c > index 6794e53c0b..e1e06d23cc 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, > goto fail_options; > } > > - if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) || > - bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs), > - PREALLOC_MODE_OFF, NULL) != 0) { > + if (!bdrv_has_zero_init(bs->file->bs)) { > s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; > } > >
On 08/04/2017 10:32 PM, Eric Blake wrote: > On 08/04/2017 10:10 AM, Denis V. Lunev wrote: >> This would be actually strange and error prone. If truncate() nowadays >> will fail, there is something fatally wrong. Let's check for that during >> the actual work. >> >> The only fallback case is when the file is not zero initialized. In this >> case we should switch to preallocation via fallocate(). > I got confused by the commit message. Here's my attempt an an > alternative, to see if I'm understanding the point of this patch: > > The code was trying to truncate a file to its current length, as an > optimization to help decide whether an alternative prealloc mode was > useful. But it forgot to check whether bdrv_getlength() succeeded, and > dealing with that failure just complicates what was supposed to be a > no-op probe for optimizing later operation. We will still properly fail > later when an actual truncation attempt is made and the device can't > support it. So when deciding how to set prealloc_mode while opening the > device, all we really need is to check just the one condition that > matters - knowing whether the device is zero initialized. > This is not an optimization. This is check that truncate is working for underlying BDS. 2 years ago I though that I have seen some BDSes without truncate support. So, with this patch I have dropped this check. If the user has specified to change preallocation mode, he is responsible to be correct. If truncate is broken, this will be revealed at the first attempt to expand the file. >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Markus Armbruster <armbru@redhat.com> >> CC: Kevin Wolf <kwolf@redhat.com> >> CC: Max Reitz <mreitz@redhat.com> >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> block/parallels.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) > The code change makes sense at first glance, but I'm a bit reluctant to > give R-b, since the commit message threw me off and I'm not familiar > with the parallels code in the first place. > >> diff --git a/block/parallels.c b/block/parallels.c >> index 6794e53c0b..e1e06d23cc 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c >> @@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, >> goto fail_options; >> } >> >> - if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) || >> - bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs), >> - PREALLOC_MODE_OFF, NULL) != 0) { >> + if (!bdrv_has_zero_init(bs->file->bs)) { >> s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; >> } >> >>
diff --git a/block/parallels.c b/block/parallels.c index 6794e53c0b..e1e06d23cc 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, goto fail_options; } - if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) || - bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs), - PREALLOC_MODE_OFF, NULL) != 0) { + if (!bdrv_has_zero_init(bs->file->bs)) { s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; }
This would be actually strange and error prone. If truncate() nowadays will fail, there is something fatally wrong. Let's check for that during the actual work. The only fallback case is when the file is not zero initialized. In this case we should switch to preallocation via fallocate(). Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Markus Armbruster <armbru@redhat.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Max Reitz <mreitz@redhat.com> CC: Stefan Hajnoczi <stefanha@redhat.com> --- block/parallels.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)