| Message ID | 1477845724-27586-3-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Oct 30, 2016 at 11:42:02AM -0500, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
This doesn't belong in this patchset.
Regardless, can we just implement the damned thing rather than
removing it? Plenty of people have asked for it and they still want
this functionality. I've sent a couple of different prototypes that
worked but got bikeshedded to death, and IIRC Ben also tried to get
it implemented but that went nowhere because other parts of his
patchset got bikeshedded to death.
If nothing else, just let me implement it in XFS like I did the
first time so when all the bikshedding stops we can convert it to
the One True AIO Interface that is decided on.
Cheers,
Dave.
On Mon, Oct 31, 2016 at 10:23:31AM +1100, Dave Chinner wrote: > This doesn't belong in this patchset. It does. I can't fix up the calling conventions for a methods that was never implemented. > Regardless, can we just implement the damned thing rather than > removing it? Plenty of people have asked for it and they still want > this functionality. I've sent a couple of different prototypes that > worked but got bikeshedded to death, and IIRC Ben also tried to get > it implemented but that went nowhere because other parts of his > patchset got bikeshedded to death. > > If nothing else, just let me implement it in XFS like I did the > first time so when all the bikshedding stops we can convert it to > the One True AIO Interface that is decided on. I'm not going to complain about a proper implementation, but right now we don't have any, and I'm not even sure the method signature is all that suitable. E.g. for the in-kernel users we'd really want a ranged fsync like the normal fsync anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 31, 2016 at 02:07:54PM +0100, Christoph Hellwig wrote: > On Mon, Oct 31, 2016 at 10:23:31AM +1100, Dave Chinner wrote: > > This doesn't belong in this patchset. > > It does. I can't fix up the calling conventions for a methods that > was never implemented. That sounds like a problem with your fix - it should work regardless of whether a valid/implemented AIO function is called or not, right? There's no difference between an invalid command, IOCB_CMD_FSYNC where ->aio_fsync() is null, or some supported command that immediately returns -EIO, the end result should be the same... > > Regardless, can we just implement the damned thing rather than > > removing it? Plenty of people have asked for it and they still want > > this functionality. I've sent a couple of different prototypes that > > worked but got bikeshedded to death, and IIRC Ben also tried to get > > it implemented but that went nowhere because other parts of his > > patchset got bikeshedded to death. > > > > If nothing else, just let me implement it in XFS like I did the > > first time so when all the bikshedding stops we can convert it to > > the One True AIO Interface that is decided on. > > I'm not going to complain about a proper implementation, but right now > we don't have any, and I'm not even sure the method signature is > all that suitable. E.g. for the in-kernel users we'd really want a > ranged fsync like the normal fsync anyway. You mean like this version I posted a year ago: https://lkml.org/lkml/2015/10/29/517 Cheers, Dave.
On Mon, Oct 31, 2016 at 1:25 PM, Dave Chinner <david@fromorbit.com> wrote: > > You mean like this version I posted a year ago: > > https://lkml.org/lkml/2015/10/29/517 I still suspect that if we want to do this, we should strive to expose all the other syncing flags from sync_file_range() too. Not everybody wants to do just synchronous syncs. Especially if you're doing async work, you might well want to have one async operation to *start* the writeback on a range, then do something else, and then do one to wait for the sync to actually have succeeded. Yeah, that's more of a "keep writes streaming" interface than a fsync() like interface, but I think the two really do fit together. It's kind of sad how we have this very fragmented interface to writeback, where some operations take that "data vs metadata", some operations take a range of bytes, and some operations take that "start writeback vs wait for it", but nothing does all of the above. They are really just different faces of the same writeback coin. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 01, 2016 at 07:25:21AM +1100, Dave Chinner wrote: > That sounds like a problem with your fix - it should work > regardless of whether a valid/implemented AIO function is called > or not, right? There's no difference between an invalid command, > IOCB_CMD_FSYNC where ->aio_fsync() is null, or some supported > command that immediately returns -EIO, the end result should > be the same... We would need the same increased file refcount if aio_fsync actually was implemented using -EIOCBQUEUED returns. We wouldn't nessecarily need it without that. > > I'm not going to complain about a proper implementation, but right now > > we don't have any, and I'm not even sure the method signature is > > all that suitable. E.g. for the in-kernel users we'd really want a > > ranged fsync like the normal fsync anyway. > > You mean like this version I posted a year ago: > > https://lkml.org/lkml/2015/10/29/517 I'd love to see that one in - but it doesn't use the aio_fsync method either.. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 31, 2016 at 06:30:11PM -0700, Linus Torvalds wrote: > I still suspect that if we want to do this, we should strive to expose > all the other syncing flags from sync_file_range() too. sync_file_range is entirely different from fsync - sync_file_range allows you detailed control of data writeback, but it does not allow to commit metadata at all, i.e. it's not a data integrity operation. > Yeah, that's more of a "keep writes streaming" interface than a > fsync() like interface, but I think the two really do fit together. > It's kind of sad how we have this very fragmented interface to > writeback, where some operations take that "data vs metadata", some > operations take a range of bytes, and some operations take that "start > writeback vs wait for it", but nothing does all of the above. They are > really just different faces of the same writeback coin. sync_file_range at the moment actually doesn't involve the fs, which has it's own set of problems. So yes, maybe we need a full blown sync method unifying fsync, sync_file_range and which enables ranged data integrity fsync and asynchronous operations of all this. But to go back to Dave's argument - none of that can archived with that aio_fsync method added more than 10 years ago and never implemented. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 1, 2016 at 7:30 AM, Christoph Hellwig <hch@lst.de> wrote: > > sync_file_range is entirely different from fsync - Absolutely. And I think it's a shame. Particularly the fact that we don't have any way to actually tie into the filesystem, and we just work on the page cache level, which "mostly works" for writeback, but is really not right. It's not like a filesystem even has to work on that level at all.. > sync_file_range at the moment actually doesn't involve the fs, which > has it's own set of problems. So yes, maybe we need a full blown > sync method unifying fsync, sync_file_range and which enables ranged > data integrity fsync and asynchronous operations of all this. I *think* we could just expand the existing filesystem "f_op->fsync()" interface to take more than just the single-bit argument, and have the full interface. But being Linux-only, it sadly will never get much use. Even if the other flags really are very useful for getting overlapping streaming writes with minimal dirty state. It might be more productive to have some easier-to-use interface to some generic writebehind logic, but.. > But to go back to Dave's argument - none of that can archived with that > aio_fsync method added more than 10 years ago and never implemented. Yeah, no, I actually agree with just removing that. I think we are often much too slow to just say "nobody uses it, get rid of it". Even if it were to ever be needed in the future, we could just resurrect it, but it's not clear that it would ever be in that particular form. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking index 14cdc10..1b5f156 100644 --- a/Documentation/filesystems/Locking +++ b/Documentation/filesystems/Locking @@ -447,7 +447,6 @@ prototypes: int (*flush) (struct file *); int (*release) (struct inode *, struct file *); int (*fsync) (struct file *, loff_t start, loff_t end, int datasync); - int (*aio_fsync) (struct kiocb *, int datasync); int (*fasync) (int, struct file *, int); int (*lock) (struct file *, int, struct file_lock *); ssize_t (*readv) (struct file *, const struct iovec *, unsigned long, diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index d619c8d..b5039a0 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -828,7 +828,6 @@ struct file_operations { int (*flush) (struct file *, fl_owner_t id); int (*release) (struct inode *, struct file *); int (*fsync) (struct file *, loff_t, loff_t, int datasync); - int (*aio_fsync) (struct kiocb *, int datasync); int (*fasync) (int, struct file *, int); int (*lock) (struct file *, int, struct file_lock *); ssize_t (*sendpage) (struct file *, struct page *, int, size_t, loff_t *, int); diff --git a/fs/aio.c b/fs/aio.c index 0aa71d3..2a6030a 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -1472,20 +1472,6 @@ static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode, kfree(iovec); break; - case IOCB_CMD_FDSYNC: - if (!file->f_op->aio_fsync) - return -EINVAL; - - ret = file->f_op->aio_fsync(req, 1); - break; - - case IOCB_CMD_FSYNC: - if (!file->f_op->aio_fsync) - return -EINVAL; - - ret = file->f_op->aio_fsync(req, 0); - break; - default: pr_debug("EINVAL: no operation provided\n"); return -EINVAL; diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c index a186135..0ee19ec 100644 --- a/fs/ntfs/dir.c +++ b/fs/ntfs/dir.c @@ -1544,8 +1544,6 @@ const struct file_operations ntfs_dir_ops = { .iterate = ntfs_readdir, /* Read directory contents. */ #ifdef NTFS_RW .fsync = ntfs_dir_fsync, /* Sync a directory to disk. */ - /*.aio_fsync = ,*/ /* Sync all outstanding async - i/o operations on a kiocb. */ #endif /* NTFS_RW */ /*.ioctl = ,*/ /* Perform function on the mounted filesystem. */ diff --git a/include/linux/fs.h b/include/linux/fs.h index 16d2b6e..ff7bcd9 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1709,7 +1709,6 @@ struct file_operations { int (*flush) (struct file *, fl_owner_t id); int (*release) (struct inode *, struct file *); int (*fsync) (struct file *, loff_t, loff_t, int datasync); - int (*aio_fsync) (struct kiocb *, int datasync); int (*fasync) (int, struct file *, int); int (*lock) (struct file *, int, struct file_lock *); ssize_t (*sendpage) (struct file *, struct page *, int, size_t, loff_t *, int);
Signed-off-by: Christoph Hellwig <hch@lst.de> --- Documentation/filesystems/Locking | 1 - Documentation/filesystems/vfs.txt | 1 - fs/aio.c | 14 -------------- fs/ntfs/dir.c | 2 -- include/linux/fs.h | 1 - 5 files changed, 19 deletions(-)