Message ID | 1522204637-29589-1-git-send-email-zhenwei.pi@youruncloud.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 03/27/2018 09:37 PM, zhenwei.pi wrote: > since linux 4.9, block device supports fallocate. kernel issues > block device zereout request and invalidates page cache. So > ioctl(fd, FALLOC_FL_ZERO_RANGE...) is safer than ioctl(fd, did you mean fallocate() in the first half of the sentence? > BLKZEROOUT...). try to call do_fallocate, if failing, fallback. > > use new field "has_fallocate_zero_range" with default value as > true. if do_fallocate returns -ENOTSUP, it will be set false. > > Signed-off-by: zhenwei.pi <zhenwei.pi@youruncloud.com> > --- > block/file-posix.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > This feels more like a feature for 2.13, than a bug fix that would fit during freeze for 2.12. > diff --git a/block/file-posix.c b/block/file-posix.c > index d7fb772..842e940 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -159,8 +159,9 @@ typedef struct BDRVRawState { > bool discard_zeroes:1; > bool use_linux_aio:1; > bool page_cache_inconsistent:1; > - bool has_fallocate; > - bool needs_alignment; > + bool has_fallocate:1; > + bool has_fallocate_zero_range:1; > + bool needs_alignment:1; > > PRManager *pr_mgr; > } BDRVRawState; > @@ -549,6 +550,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > > s->has_discard = true; > s->has_write_zeroes = true; > + s->has_fallocate_zero_range = true; Is blindly setting this to true reasonable, given that... > if ((bs->open_flags & BDRV_O_NOCACHE) != 0) { > s->needs_alignment = true; > } > @@ -1365,10 +1367,6 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) > int64_t len; > #endif > > - if (aiocb->aio_type & QEMU_AIO_BLKDEV) { > - return handle_aiocb_write_zeroes_block(aiocb); > - } > - > #ifdef CONFIG_XFS > if (s->is_xfs) { > return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes); > @@ -1376,16 +1374,25 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) > #endif > > #ifdef CONFIG_FALLOCATE_ZERO_RANGE > - if (s->has_write_zeroes) { ...later use is guarded by something learned at compile time? > + /* since linux 4.9, block device supports fallocate. kernel issues s/since/Since/ s/device supports/devices support/ s/kernel issues/The kernel issues a/ > + * block device zereout request and invalidates page cache. So > + * ioctl(fd, FALLOC_FL_ZERO_RANGE...) is safer than ioctl(fd, Same comment as on commit message; this looks like you meant fallocate rather than ioctl on one of the two uses. > + * BLKZEROOUT...). try to call do_fallocate, if failing, fallback. s/try/Try/ s/if failing, fallback/and fall back if that fails/ > + */ > + if (s->has_fallocate_zero_range) { > int ret = do_fallocate(s->fd, FALLOC_FL_ZERO_RANGE, > aiocb->aio_offset, aiocb->aio_nbytes); > - if (ret == 0 || ret != -ENOTSUP) { > + if (ret == 0) { > return ret; > - } > - s->has_write_zeroes = false; > + } else if (ret == -ENOTSUP) > + s->has_fallocate_zero_range = false; > } Before your patch, if we get any failure other than -ENOTSUP, we exit immediately rather than attempting a fallback. Your code breaks that paradigm, and blindly attempts the fallback even when the failure was something like EIO. > #endif > > + if (aiocb->aio_type & QEMU_AIO_BLKDEV) { > + return handle_aiocb_write_zeroes_block(aiocb); > + } > + > #ifdef CONFIG_FALLOCATE_PUNCH_HOLE > if (s->has_discard && s->has_fallocate) { > int ret = do_fallocate(s->fd, >
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 1522204637-29589-1-git-send-email-zhenwei.pi@youruncloud.com Subject: [Qemu-devel] [PATCH] file-posix: Support fallocate for block device === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' c955c08515 file-posix: Support fallocate for block device === OUTPUT BEGIN === Checking PATCH 1/1: file-posix: Support fallocate for block device... ERROR: braces {} are necessary for all arms of this statement #66: FILE: block/file-posix.c:1385: + if (ret == 0) { [...] - } [...] ERROR: braces {} are necessary for all arms of this statement #70: FILE: block/file-posix.c:1387: + } else if (ret == -ENOTSUP) [...] total: 2 errors, 0 warnings, 57 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
diff --git a/block/file-posix.c b/block/file-posix.c index d7fb772..842e940 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -159,8 +159,9 @@ typedef struct BDRVRawState { bool discard_zeroes:1; bool use_linux_aio:1; bool page_cache_inconsistent:1; - bool has_fallocate; - bool needs_alignment; + bool has_fallocate:1; + bool has_fallocate_zero_range:1; + bool needs_alignment:1; PRManager *pr_mgr; } BDRVRawState; @@ -549,6 +550,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, s->has_discard = true; s->has_write_zeroes = true; + s->has_fallocate_zero_range = true; if ((bs->open_flags & BDRV_O_NOCACHE) != 0) { s->needs_alignment = true; } @@ -1365,10 +1367,6 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) int64_t len; #endif - if (aiocb->aio_type & QEMU_AIO_BLKDEV) { - return handle_aiocb_write_zeroes_block(aiocb); - } - #ifdef CONFIG_XFS if (s->is_xfs) { return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes); @@ -1376,16 +1374,25 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) #endif #ifdef CONFIG_FALLOCATE_ZERO_RANGE - if (s->has_write_zeroes) { + /* since linux 4.9, block device supports fallocate. kernel issues + * block device zereout request and invalidates page cache. So + * ioctl(fd, FALLOC_FL_ZERO_RANGE...) is safer than ioctl(fd, + * BLKZEROOUT...). try to call do_fallocate, if failing, fallback. + */ + if (s->has_fallocate_zero_range) { int ret = do_fallocate(s->fd, FALLOC_FL_ZERO_RANGE, aiocb->aio_offset, aiocb->aio_nbytes); - if (ret == 0 || ret != -ENOTSUP) { + if (ret == 0) { return ret; - } - s->has_write_zeroes = false; + } else if (ret == -ENOTSUP) + s->has_fallocate_zero_range = false; } #endif + if (aiocb->aio_type & QEMU_AIO_BLKDEV) { + return handle_aiocb_write_zeroes_block(aiocb); + } + #ifdef CONFIG_FALLOCATE_PUNCH_HOLE if (s->has_discard && s->has_fallocate) { int ret = do_fallocate(s->fd,
since linux 4.9, block device supports fallocate. kernel issues block device zereout request and invalidates page cache. So ioctl(fd, FALLOC_FL_ZERO_RANGE...) is safer than ioctl(fd, BLKZEROOUT...). try to call do_fallocate, if failing, fallback. use new field "has_fallocate_zero_range" with default value as true. if do_fallocate returns -ENOTSUP, it will be set false. Signed-off-by: zhenwei.pi <zhenwei.pi@youruncloud.com> --- block/file-posix.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)