| Message ID | 20191212163904.159893-98-dgilbert@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofs daemon [all] | expand |
On Thu, Dec 12, 2019 at 04:38:57PM +0000, Dr. David Alan Gilbert (git) wrote: > From: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> > > When writeback mode is enabled (-o writeback), O_APPEND handling is > done in kernel. Therefore virtiofsd clears O_APPEND flag when open. > Otherwise O_APPEND flag takes precedence over pwrite() and write > data may corrupt. > > Currently clearing O_APPEND flag is done in lo_open(), but we also > need the same operation in lo_create(). So, factor out the flag > update operation in lo_open() to update_open_flags() and call it > in both lo_open() and lo_create(). > > This fixes the failure of xfstest generic/069 in writeback mode > (which tests O_APPEND write data integrity). Seeing this mention of xfstest reminds me that there are no tests added anywhere in this patch series. Is there another followup pending with tests or is it a todo item ? We can usefully wire up this xfstest program in the functional tests of QEMU in some way ? > > Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> > --- > tools/virtiofsd/passthrough_ll.c | 66 ++++++++++++++++---------------- > 1 file changed, 33 insertions(+), 33 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Thu, Dec 12, 2019 at 04:38:57PM +0000, Dr. David Alan Gilbert (git) wrote: > > From: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> > > > > When writeback mode is enabled (-o writeback), O_APPEND handling is > > done in kernel. Therefore virtiofsd clears O_APPEND flag when open. > > Otherwise O_APPEND flag takes precedence over pwrite() and write > > data may corrupt. > > > > Currently clearing O_APPEND flag is done in lo_open(), but we also > > need the same operation in lo_create(). So, factor out the flag > > update operation in lo_open() to update_open_flags() and call it > > in both lo_open() and lo_create(). > > > > This fixes the failure of xfstest generic/069 in writeback mode > > (which tests O_APPEND write data integrity). > > Seeing this mention of xfstest reminds me that there are no tests > added anywhere in this patch series. Is there another followup > pending with tests or is it a todo item ? We've got some github CI setup, but not too much automatic of as yet. As you spotted in another patch we need root to run this at the moment which makes life harder; we also need a full guest to drive fuse requests. > We can usefully wire up this xfstest program in the functional > tests of QEMU in some way ? > > > > > Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> > > --- > > tools/virtiofsd/passthrough_ll.c | 66 ++++++++++++++++---------------- > > 1 file changed, 33 insertions(+), 33 deletions(-) > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Thanks. > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 6b3d396b6f..1bf251a91d 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -1676,6 +1676,37 @@ static void lo_releasedir(fuse_req_t req, fuse_ino_t ino, fuse_reply_err(req, 0); } +static void update_open_flags(int writeback, struct fuse_file_info *fi) +{ + /* + * With writeback cache, kernel may send read requests even + * when userspace opened write-only + */ + if (writeback && (fi->flags & O_ACCMODE) == O_WRONLY) { + fi->flags &= ~O_ACCMODE; + fi->flags |= O_RDWR; + } + + /* + * With writeback cache, O_APPEND is handled by the kernel. + * This breaks atomicity (since the file may change in the + * underlying filesystem, so that the kernel's idea of the + * end of the file isn't accurate anymore). In this example, + * we just accept that. A more rigorous filesystem may want + * to return an error here + */ + if (writeback && (fi->flags & O_APPEND)) { + fi->flags &= ~O_APPEND; + } + + /* + * O_DIRECT in guest should not necessarily mean bypassing page + * cache on host as well. If somebody needs that behavior, it + * probably should be a configuration knob in daemon. + */ + fi->flags &= ~O_DIRECT; +} + static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, mode_t mode, struct fuse_file_info *fi) { @@ -1705,12 +1736,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, goto out; } - /* - * O_DIRECT in guest should not necessarily mean bypassing page - * cache on host as well. If somebody needs that behavior, it - * probably should be a configuration knob in daemon. - */ - fi->flags &= ~O_DIRECT; + update_open_flags(lo->writeback, fi); fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW, mode); @@ -1920,33 +1946,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino, fi->flags); - /* - * With writeback cache, kernel may send read requests even - * when userspace opened write-only - */ - if (lo->writeback && (fi->flags & O_ACCMODE) == O_WRONLY) { - fi->flags &= ~O_ACCMODE; - fi->flags |= O_RDWR; - } - - /* - * With writeback cache, O_APPEND is handled by the kernel. - * This breaks atomicity (since the file may change in the - * underlying filesystem, so that the kernel's idea of the - * end of the file isn't accurate anymore). In this example, - * we just accept that. A more rigorous filesystem may want - * to return an error here - */ - if (lo->writeback && (fi->flags & O_APPEND)) { - fi->flags &= ~O_APPEND; - } - - /* - * O_DIRECT in guest should not necessarily mean bypassing page - * cache on host as well. If somebody needs that behavior, it - * probably should be a configuration knob in daemon. - */ - fi->flags &= ~O_DIRECT; + update_open_flags(lo->writeback, fi); sprintf(buf, "%i", lo_fd(req, ino)); fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);