Re: [PATCH v2 4/7] btrfs-progs: introduce btrfs_pwrite wrapper for pwrite
From: David Sterba <hidden>
Date: 2021-10-06 14:10:26
On Tue, Oct 05, 2021 at 03:23:02PM +0900, Naohiro Aota wrote:
-int device_zero_blocks(int fd, off_t start, size_t len) +int device_zero_blocks(int fd, off_t start, size_t len, const bool direct)
const for ints or bools does not make sense, the const for pointers is an API contract that the callee won't change it but for local variables defined inside parameter list it's not necessary.
quoted hunk ↗ jump to hunk
{ char *buf = malloc(len); int ret = 0;@@ -104,7 +105,7 @@ int device_zero_blocks(int fd, off_t start, size_t len) if (!buf) return -ENOMEM; memset(buf, 0, len); - written = pwrite(fd, buf, len, start); + written = btrfs_pwrite(fd, buf, len, start, direct); if (written != len) ret = -EIO; free(buf);@@ -134,7 +135,7 @@ static int zero_dev_clamped(int fd, struct btrfs_zoned_device_info *zinfo, if (zinfo && zinfo->model == ZONED_HOST_MANAGED) return zero_zone_blocks(fd, zinfo, start, end - start); - return device_zero_blocks(fd, start, end - start); + return device_zero_blocks(fd, start, end - start, false); } /*@@ -176,8 +177,10 @@ static int btrfs_wipe_existing_sb(int fd, struct btrfs_zoned_device_info *zinfo) len = sizeof(buf); if (!zone_is_sequential(zinfo, offset)) { + const bool direct = zinfo && zinfo->model == ZONED_HOST_MANAGED; + memset(buf, 0, len); - ret = pwrite(fd, buf, len, offset); + ret = btrfs_pwrite(fd, buf, len, offset, direct); if (ret < 0) {
This should probably also check for ret == 0 as this does nothing, but that's a separate fix.
quoted hunk ↗ jump to hunk
error("cannot wipe existing superblock: %m"); ret = -1;@@ -510,3 +513,68 @@ out: close(sysfs_fd); return ret; } + +ssize_t btrfs_direct_pio(int rw, int fd, void *buf, size_t count, off_t offset) +{ + int alignment; + size_t iosize; + void *bounce_buf = NULL; + struct stat stat_buf; + unsigned long req; + int ret; + ssize_t ret_rw; + + ASSERT(rw == READ || rw == WRITE); + + if (fstat(fd, &stat_buf) == -1) { + error("fstat failed (%m)"); + return 0; + } + + if ((stat_buf.st_mode & S_IFMT) == S_IFBLK) + req = BLKSSZGET; + else + req = FIGETBSZ;
+
+ if (ioctl(fd, req, &alignment)) {
+ error("failed to get block size: %m");
+ return 0;The ioctls take an int as parameter but it's not well suitable for alignment checks as it internally does bit operations and this should be avoided. It should work here but would be good to clean it up.
+ }
+
+ if (IS_ALIGNED((size_t)buf, alignment) && IS_ALIGNED(count, alignment)) {
+ if (rw == WRITE)
+ return pwrite(fd, buf, count, offset);
+ else
+ return pread(fd, buf, count, offset);
+ }
+
+ /* Cannot do anything if the write size is not aligned */
+ if (rw == WRITE && !IS_ALIGNED(count, alignment)) {
+ error("%lu is not aligned to %d", count, alignment);as count is size_t, the format specifier should be %zu
+ return 0;
+ }
+
+ iosize = round_up(count, alignment);
+
+ ret = posix_memalign(&bounce_buf, alignment, iosize);
+ if (ret) {
+ error("failed to allocate bounce buffer: %m");
+ errno = ret;
+ return 0;
+ }
+
+ if (rw == WRITE) {
+ ASSERT(iosize == count);
+ memcpy(bounce_buf, buf, count);
+ ret_rw = pwrite(fd, bounce_buf, iosize, offset);
+ } else {
+ ret_rw = pread(fd, bounce_buf, iosize, offset);
+ if (ret_rw >= count) {
+ ret_rw = count;
+ memcpy(buf, bounce_buf, count);
+ }
+ }
+
+ free(bounce_buf);The wrappers should entirely copy the semantics of pwrite/pread, ie return <0 on error as -errno in case of error, 0 if there was no write and otherwise the number of written bytes. I'll do the minor fixups only, the pwrite cleanups need to audit all call sites so it's better for a separate patch.