Skip to contentSkip to conversation

raw: fsync method is now required

PATCH In review Anton Blanchard 5 participants 6 messages Apr 4, 2010 – Apr 6, 2010

Subsystems (2)
VFS Char and misc drivers
lore
Share & tools
lore

Anton Blanchard

author
#0 lore raw

PATCH 1/1 raw: fsync method is now required

Commit 148f948ba877f4d3cdef036b1ff6d9f68986706a (vfs: Introduce new helpers for syncing after writing to O_SYNC file or IS_SYNC inode) broke the raw driver.

We now call through generic_file_aio_write -> generic_write_sync -> vfs_fsync_range. vfs_fsync_range has:

        if (!fop || !fop->fsync) {
                ret = -EINVAL;
                goto out;
        }

But drivers/char/raw.c doesn't set an fsync method.

We have two options: fix it or remove the raw driver completely. I'm happy to do either, the fact this has been broken for so long suggests it is rarely used.

The patch below adds an fsync method to the raw driver. My knowledge of the block layer is pretty sketchy so this could do with a once over.

If we instead decide to remove the raw driver, this patch might still be useful as a backport to 2.6.33 and 2.6.32.

Signed-off-by Anton Blanchard <[email protected]>

---

drivers/char/raw.c +1 -0
Index: linux-2.6/drivers/char/raw.c
===================================================================
@@ -247,6 +247,7 @@ static const struct file_operations raw_
 	.aio_read = 	generic_file_aio_read,
 	.write	=	do_sync_write,
 	.aio_write =	blkdev_aio_write,
+	.fsync	=	block_fsync,
 	.open	=	raw_open,
 	.release=	raw_release,
 	.ioctl	=	raw_ioctl,
fs/block_dev.c +2 -1
Index: linux-2.6/fs/block_dev.c
===================================================================
@@ -404,7 +404,7 @@ static loff_t block_llseek(struct file *
  *	NULL first argument is nfsd_sync_dir() and that's not a directory.
  */
  
-static int block_fsync(struct file *filp, struct dentry *dentry, int datasync)
+int block_fsync(struct file *filp, struct dentry *dentry, int datasync)
 {
 	struct block_device *bdev = I_BDEV(filp->f_mapping->host);
 	int error;

@@ -418,6 +418,7 @@ static int block_fsync(struct file *filp
 		error = 0;
 	return error;
 }
+EXPORT_SYMBOL(block_fsync);
 
 /*
  * pseudo-fs
include/linux/fs.h +1 -0
Index: linux-2.6/include/linux/fs.h
===================================================================
@@ -2212,6 +2212,7 @@ extern int generic_segment_checks(const 
 /* fs/block_dev.c */
 extern ssize_t blkdev_aio_write(struct kiocb *iocb, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos);
+extern int block_fsync(struct file *filp, struct dentry *dentry, int datasync);
 
 /* fs/splice.c */
 extern ssize_t generic_file_splice_read(struct file *, loff_t *,
#1 lore raw
Quoted 6 lines · On Sun, 4 Apr 2010 10:45:23 +1000 Anton Blanchard <[email protected]> wrote:

Commit 148f948ba877f4d3cdef036b1ff6d9f68986706a (vfs: Introduce new helpers for syncing after writing to O_SYNC file or IS_SYNC inode) broke the raw driver.

We now call through generic_file_aio_write -> generic_write_sync -> vfs_fsync_range. vfs_fsync_range has:

Quoted 6 lines
        if (!fop || !fop->fsync) {
                ret = -EINVAL;
                goto out;
        }

But drivers/char/raw.c doesn't set an fsync method.

So if you run fsync() against a /dev/rawX fd you get -EINVAL? erk.

Quoted 3 lines

We have two options: fix it or remove the raw driver completely. I'm happy to do either, the fact this has been broken for so long suggests it is rarely used.

Well. It shows that fsync() is rarely used.

Quoted 5 lines

The patch below adds an fsync method to the raw driver. My knowledge of the block layer is pretty sketchy so this could do with a once over.

If we instead decide to remove the raw driver, this patch might still be useful as a backport to 2.6.33 and 2.6.32.

People go all whiny when I talk about removing raw.c. I gave up.

#2 lore raw
Quoted 7 lines · Andrew Morton

So if you run fsync() against a /dev/rawX fd you get -EINVAL? erk.

Quoted 3 lines

We have two options: fix it or remove the raw driver completely. I'm happy to do either, the fact this has been broken for so long suggests it is rarely used.

Well. It shows that fsync() is rarely used.

The new O_SYNC implementation uses fsync, so O_SYNC users are also affected now. Still shows that either one is relatively rarely used..

Reviewed-by Tested-by on v1 1/1
#3 lore raw
In reply to Anton Blanchard
Quoted 16 lines · Anton Blanchard

Commit 148f948ba877f4d3cdef036b1ff6d9f68986706a (vfs: Introduce new helpers for syncing after writing to O_SYNC file or IS_SYNC inode) broke the raw driver.

We now call through generic_file_aio_write -> generic_write_sync -> vfs_fsync_range. vfs_fsync_range has:

        if (!fop || !fop->fsync) {
                ret = -EINVAL;
                goto out;
        }

But drivers/char/raw.c doesn't set an fsync method.

We have two options: fix it or remove the raw driver completely. I'm happy to do either, the fact this has been broken for so long suggests it is rarely used.

Quoted 5 lines

The patch below adds an fsync method to the raw driver. My knowledge of the block layer is pretty sketchy so this could do with a once over.

If we instead decide to remove the raw driver, this patch might still be useful as a backport to 2.6.33 and 2.6.32.

Databases like to use the raw interface. Thanks for fixing this!

Quoted 1 line
Signed-off-by Anton Blanchard <[email protected]>
Reviewed-by Jeff Moyer <[email protected]>
Tested-by Jeff Moyer <[email protected]>
reviewer Reviewed-by on v1 1/1
#4 lore raw
In reply to Anton Blanchard
Quoted 10 lines · Anton Blanchard

Commit 148f948ba877f4d3cdef036b1ff6d9f68986706a (vfs: Introduce new helpers for syncing after writing to O_SYNC file or IS_SYNC inode) broke the raw driver.

We now call through generic_file_aio_write -> generic_write_sync -> vfs_fsync_range. vfs_fsync_range has:

        if (!fop || !fop->fsync) {
                ret = -EINVAL;
                goto out;
        }
Quoted 8 lines

But drivers/char/raw.c doesn't set an fsync method.

We have two options: fix it or remove the raw driver completely. I'm happy to do either, the fact this has been broken for so long suggests it is rarely used.

The patch below adds an fsync method to the raw driver. My knowledge of the block layer is pretty sketchy so this could do with a once over.

Thanks for catching this. The patch looks fine.

Quoted 2 lines

If we instead decide to remove the raw driver, this patch might still be useful as a backport to 2.6.33 and 2.6.32.

Oracle databases still use raw devices so I don't think we should remove
them.

Reviewed-by Jan Kara <[email protected]>

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR