| Message ID | 20181203101429.88735-7-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: cluster space preallocation | expand |
03.12.2018 13:14, Anton Nefedov wrote: > Current write_zeroes implementation is good enough to satisfy this flag too > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > --- > block/file-posix.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 07bbdab953..b0b7ab0159 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -603,6 +603,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > } else { > s->discard_zeroes = true; > s->has_fallocate = true; > + bs->supported_zero_flags = BDRV_REQ_ALLOCATE; > } > } else { > if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) { > @@ -646,10 +647,11 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > #ifdef CONFIG_XFS > if (platform_test_xfs_fd(s->fd)) { > s->is_xfs = true; > + bs->supported_zero_flags = BDRV_REQ_ALLOCATE; why we should handle xfs separately? is there a case with xfs, not handled by previous generic if ()? > } > #endif > > - bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP; > + bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP; > ret = 0; > fail: > if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) { > @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) > } > s->has_fallocate = false; > } > + > + if (!s->has_fallocate) { > + aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE; > + } > #endif > > return -ENOTSUP; >
On 5/12/2018 4:25 PM, Vladimir Sementsov-Ogievskiy wrote: > 03.12.2018 13:14, Anton Nefedov wrote: >> Current write_zeroes implementation is good enough to satisfy this flag too >> >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >> --- >> block/file-posix.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 07bbdab953..b0b7ab0159 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -603,6 +603,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, >> } else { >> s->discard_zeroes = true; >> s->has_fallocate = true; >> + bs->supported_zero_flags = BDRV_REQ_ALLOCATE; >> } >> } else { >> if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) { >> @@ -646,10 +647,11 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, >> #ifdef CONFIG_XFS >> if (platform_test_xfs_fd(s->fd)) { >> s->is_xfs = true; >> + bs->supported_zero_flags = BDRV_REQ_ALLOCATE; > > why we should handle xfs separately? is there a case with xfs, not handled by previous generic if ()? > The driver supports ALLOCATE either when it's XFS, or when fallocate is available. Currently there is no test for fallocate, it's just implied it's supported until ENOTSUP returned. I think it's safer (for possible future changes) to set it twice even though you're right and first condition currently covers the XFS condition too. >> } >> #endif >> >> - bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP; >> + bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP; >> ret = 0; >> fail: >> if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) { >> @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) >> } >> s->has_fallocate = false; >> } >> + >> + if (!s->has_fallocate) { >> + aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE; >> + } >> #endif >> >> return -ENOTSUP; >> > >
On Mon 03 Dec 2018 11:14:58 AM CET, Anton Nefedov wrote: > Current write_zeroes implementation is good enough to satisfy this flag too > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
05.12.2018 17:11, Anton Nefedov wrote: > On 5/12/2018 4:25 PM, Vladimir Sementsov-Ogievskiy wrote: >> 03.12.2018 13:14, Anton Nefedov wrote: >>> Current write_zeroes implementation is good enough to satisfy this flag too >>> >>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >>> --- >>> block/file-posix.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/block/file-posix.c b/block/file-posix.c >>> index 07bbdab953..b0b7ab0159 100644 >>> --- a/block/file-posix.c >>> +++ b/block/file-posix.c >>> @@ -603,6 +603,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, >>> } else { >>> s->discard_zeroes = true; >>> s->has_fallocate = true; >>> + bs->supported_zero_flags = BDRV_REQ_ALLOCATE; >>> } >>> } else { >>> if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) { >>> @@ -646,10 +647,11 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, >>> #ifdef CONFIG_XFS >>> if (platform_test_xfs_fd(s->fd)) { >>> s->is_xfs = true; >>> + bs->supported_zero_flags = BDRV_REQ_ALLOCATE; >> >> why we should handle xfs separately? is there a case with xfs, not handled by previous generic if ()? >> > > The driver supports ALLOCATE either when it's XFS, or when fallocate is > available. Currently there is no test for fallocate, it's just implied > it's supported until ENOTSUP returned. > I think it's safer (for possible future changes) to set it twice even > though you're right and first condition currently covers the XFS > condition too. Aha, ok. > >>> } >>> #endif >>> >>> - bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP; >>> + bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP; >>> ret = 0; >>> fail: >>> if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) { >>> @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) >>> } >>> s->has_fallocate = false; >>> } >>> + >>> + if (!s->has_fallocate) { >>> + aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE; >>> + } hm, if CONFIG_FALLOCATE is disabled, flag will remain in supported_zero_flags >>> #endif >>> >>> return -ENOTSUP; >>> >> >>
On 12/12/2018 8:19 PM, Vladimir Sementsov-Ogievskiy wrote: > 05.12.2018 17:11, Anton Nefedov wrote: >> On 5/12/2018 4:25 PM, Vladimir Sementsov-Ogievskiy wrote: >>> 03.12.2018 13:14, Anton Nefedov wrote: >>>> } >>>> #endif >>>> >>>> - bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP; >>>> + bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP; >>>> ret = 0; >>>> fail: >>>> if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) { >>>> @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) >>>> } >>>> s->has_fallocate = false; >>>> } >>>> + >>>> + if (!s->has_fallocate) { >>>> + aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE; >>>> + } >>>>> #endif > > hm, if CONFIG_FALLOCATE is disabled, flag will remain in supported_zero_flags > right.. I think there should be a separate patch to reset s->has_* in non-CONFIG_FALLOCATE* cases. Then I'll move this hunk just one line down under the following #endif
diff --git a/block/file-posix.c b/block/file-posix.c index 07bbdab953..b0b7ab0159 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -603,6 +603,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, } else { s->discard_zeroes = true; s->has_fallocate = true; + bs->supported_zero_flags = BDRV_REQ_ALLOCATE; } } else { if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) { @@ -646,10 +647,11 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, #ifdef CONFIG_XFS if (platform_test_xfs_fd(s->fd)) { s->is_xfs = true; + bs->supported_zero_flags = BDRV_REQ_ALLOCATE; } #endif - bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP; + bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP; ret = 0; fail: if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) { @@ -1520,6 +1522,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) } s->has_fallocate = false; } + + if (!s->has_fallocate) { + aiocb->bs->supported_zero_flags &= ~BDRV_REQ_ALLOCATE; + } #endif return -ENOTSUP;
Current write_zeroes implementation is good enough to satisfy this flag too Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> --- block/file-posix.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)