Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
On Wed, 2012-10-03 at 07:47 -0400, Christoph Hellwig wrote: > On Tue, Oct 02, 2012 at 01:16:44PM -0700, Nicholas A. Bellinger wrote: > > * Optionally allow fd_buffered_io=1 to be enabled for people > > * who want use the fs buffer cache as an WriteCache mechanism. > > * > > * This means that in event of a hard failure, there is a risk > > * of silent data-loss if the SCSI client has *not* performed a > > * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE > > * to write-out the entire device cache. > > */ > > Oh, I get Vlads flame. This doesn't simply disable O_DSYNC now but also > sets WCE=1. In this case I don't really get the point of the patch, why > can't we simply set it from configfs? > The patch prevents existing user-space code from using fd_buffered_io=1 operation incorrectly regardless of what is being set for emulate_write_cache device attribute. Using FILEIO w/o O_DSYNC + emulate_write_cache=0 is certainly a big no-no regardless of context, so I'd rather enforce the right thing to do here from kernel code for this special case instead of allowing an unsafe mode to be enabled by default + set attributes from user-space. --nab -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
On Tue, Oct 02, 2012 at 01:16:44PM -0700, Nicholas A. Bellinger wrote: > * Optionally allow fd_buffered_io=1 to be enabled for people > * who want use the fs buffer cache as an WriteCache mechanism. > * > * This means that in event of a hard failure, there is a risk > * of silent data-loss if the SCSI client has *not* performed a > * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE > * to write-out the entire device cache. > */ Oh, I get Vlads flame. This doesn't simply disable O_DSYNC now but also sets WCE=1. In this case I don't really get the point of the patch, why can't we simply set it from configfs? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
On Tue, Oct 02, 2012 at 01:16:44PM -0700, Nicholas A. Bellinger wrote: * Optionally allow fd_buffered_io=1 to be enabled for people * who want use the fs buffer cache as an WriteCache mechanism. * * This means that in event of a hard failure, there is a risk * of silent data-loss if the SCSI client has *not* performed a * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE * to write-out the entire device cache. */ Oh, I get Vlads flame. This doesn't simply disable O_DSYNC now but also sets WCE=1. In this case I don't really get the point of the patch, why can't we simply set it from configfs? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
On Wed, 2012-10-03 at 07:47 -0400, Christoph Hellwig wrote: On Tue, Oct 02, 2012 at 01:16:44PM -0700, Nicholas A. Bellinger wrote: * Optionally allow fd_buffered_io=1 to be enabled for people * who want use the fs buffer cache as an WriteCache mechanism. * * This means that in event of a hard failure, there is a risk * of silent data-loss if the SCSI client has *not* performed a * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE * to write-out the entire device cache. */ Oh, I get Vlads flame. This doesn't simply disable O_DSYNC now but also sets WCE=1. In this case I don't really get the point of the patch, why can't we simply set it from configfs? The patch prevents existing user-space code from using fd_buffered_io=1 operation incorrectly regardless of what is being set for emulate_write_cache device attribute. Using FILEIO w/o O_DSYNC + emulate_write_cache=0 is certainly a big no-no regardless of context, so I'd rather enforce the right thing to do here from kernel code for this special case instead of allowing an unsafe mode to be enabled by default + set attributes from user-space. --nab -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
On Mon, 2012-10-01 at 04:46 -0400, Christoph Hellwig wrote: > On Sun, Sep 30, 2012 at 05:58:11AM +, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger > > > > This patch re-adds the ability to optionally run in buffered FILEIO mode > > (eg: w/o O_DSYNC) for device backends in order to once again use the > > Linux buffered cache as a write-back storage mechanism. > > > > This difference with this patch is that fd_create_virtdevice() now > > forces the explicit setting of emulate_write_cache=1 when buffered FILEIO > > operation has been enabled. > > What this lacks is a clear reason why you would enable this inherently > unsafe mode. While there is some clear precedence to allow people doing > stupid thing I'd least like a rationale for it, and it being documented > as unsafe. > > > +/* > > Indention error. > Fixed > > +* Optionally allow fd_buffered_io=1 to be enabled for people > > +* who know what they are doing w/o O_DSYNC. > > +*/ > > This comment doesn't explain at all what's going on here. It should be > something like > > /* >* Unsafe mode allows disabling data integrity by not forcing >* data out to disk in writethrough cache mode. Only to be used >* for benchmark cheating or similar purposes. >*/ > How about the following instead..? /* * Optionally allow fd_buffered_io=1 to be enabled for people * who want use the fs buffer cache as an WriteCache mechanism. * * This means that in event of a hard failure, there is a risk * of silent data-loss if the SCSI client has *not* performed a * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE * to write-out the entire device cache. */ > > #define FBDF_HAS_PATH 0x01 > > #define FBDF_HAS_SIZE 0x02 > > +#define FDBD_USE_BUFFERED_IO 0x04 > > This should be named BDBD_UNSAFE > As we are keeping the same fd_buffered_io key=value usage that rtslib user-space already knows about, how using FDBD_HAS_BUFFERED_IO_WCE instead for consistency..? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
Christoph Hellwig, on 10/01/2012 04:46 AM wrote: On Sun, Sep 30, 2012 at 05:58:11AM +, Nicholas A. Bellinger wrote: From: Nicholas Bellinger This patch re-adds the ability to optionally run in buffered FILEIO mode (eg: w/o O_DSYNC) for device backends in order to once again use the Linux buffered cache as a write-back storage mechanism. This difference with this patch is that fd_create_virtdevice() now forces the explicit setting of emulate_write_cache=1 when buffered FILEIO operation has been enabled. What this lacks is a clear reason why you would enable this inherently unsafe mode. While there is some clear precedence to allow people doing stupid thing I'd least like a rationale for it, and it being documented as unsafe. Nowadays nearly all serious applications are transactional, and know how to flush storage cache between transactions. That means that write back caching is absolutely safe for them. No data can't be lost in any circumstances. Welcome to the 21 century, Christoph! Vlad -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
Christoph Hellwig, on 10/01/2012 04:46 AM wrote: On Sun, Sep 30, 2012 at 05:58:11AM +, Nicholas A. Bellinger wrote: From: Nicholas Bellingern...@linux-iscsi.org This patch re-adds the ability to optionally run in buffered FILEIO mode (eg: w/o O_DSYNC) for device backends in order to once again use the Linux buffered cache as a write-back storage mechanism. This difference with this patch is that fd_create_virtdevice() now forces the explicit setting of emulate_write_cache=1 when buffered FILEIO operation has been enabled. What this lacks is a clear reason why you would enable this inherently unsafe mode. While there is some clear precedence to allow people doing stupid thing I'd least like a rationale for it, and it being documented as unsafe. Nowadays nearly all serious applications are transactional, and know how to flush storage cache between transactions. That means that write back caching is absolutely safe for them. No data can't be lost in any circumstances. Welcome to the 21 century, Christoph! Vlad -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
On Mon, 2012-10-01 at 04:46 -0400, Christoph Hellwig wrote: On Sun, Sep 30, 2012 at 05:58:11AM +, Nicholas A. Bellinger wrote: From: Nicholas Bellinger n...@linux-iscsi.org This patch re-adds the ability to optionally run in buffered FILEIO mode (eg: w/o O_DSYNC) for device backends in order to once again use the Linux buffered cache as a write-back storage mechanism. This difference with this patch is that fd_create_virtdevice() now forces the explicit setting of emulate_write_cache=1 when buffered FILEIO operation has been enabled. What this lacks is a clear reason why you would enable this inherently unsafe mode. While there is some clear precedence to allow people doing stupid thing I'd least like a rationale for it, and it being documented as unsafe. +/* Indention error. Fixed +* Optionally allow fd_buffered_io=1 to be enabled for people +* who know what they are doing w/o O_DSYNC. +*/ This comment doesn't explain at all what's going on here. It should be something like /* * Unsafe mode allows disabling data integrity by not forcing * data out to disk in writethrough cache mode. Only to be used * for benchmark cheating or similar purposes. */ How about the following instead..? /* * Optionally allow fd_buffered_io=1 to be enabled for people * who want use the fs buffer cache as an WriteCache mechanism. * * This means that in event of a hard failure, there is a risk * of silent data-loss if the SCSI client has *not* performed a * forced unit access (FUA) write, or issued SYNCHRONIZE_CACHE * to write-out the entire device cache. */ #define FBDF_HAS_PATH 0x01 #define FBDF_HAS_SIZE 0x02 +#define FDBD_USE_BUFFERED_IO 0x04 This should be named BDBD_UNSAFE As we are keeping the same fd_buffered_io key=value usage that rtslib user-space already knows about, how using FDBD_HAS_BUFFERED_IO_WCE instead for consistency..? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
On Sun, Sep 30, 2012 at 05:58:11AM +, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger > > This patch re-adds the ability to optionally run in buffered FILEIO mode > (eg: w/o O_DSYNC) for device backends in order to once again use the > Linux buffered cache as a write-back storage mechanism. > > This difference with this patch is that fd_create_virtdevice() now > forces the explicit setting of emulate_write_cache=1 when buffered FILEIO > operation has been enabled. What this lacks is a clear reason why you would enable this inherently unsafe mode. While there is some clear precedence to allow people doing stupid thing I'd least like a rationale for it, and it being documented as unsafe. > + /* Indention error. > + * Optionally allow fd_buffered_io=1 to be enabled for people > + * who know what they are doing w/o O_DSYNC. > + */ This comment doesn't explain at all what's going on here. It should be something like /* * Unsafe mode allows disabling data integrity by not forcing * data out to disk in writethrough cache mode. Only to be used * for benchmark cheating or similar purposes. */ > #define FBDF_HAS_PATH0x01 > #define FBDF_HAS_SIZE0x02 > +#define FDBD_USE_BUFFERED_IO 0x04 This should be named BDBD_UNSAFE -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
On Sun, Sep 30, 2012 at 05:58:11AM +, Nicholas A. Bellinger wrote: From: Nicholas Bellinger n...@linux-iscsi.org This patch re-adds the ability to optionally run in buffered FILEIO mode (eg: w/o O_DSYNC) for device backends in order to once again use the Linux buffered cache as a write-back storage mechanism. This difference with this patch is that fd_create_virtdevice() now forces the explicit setting of emulate_write_cache=1 when buffered FILEIO operation has been enabled. What this lacks is a clear reason why you would enable this inherently unsafe mode. While there is some clear precedence to allow people doing stupid thing I'd least like a rationale for it, and it being documented as unsafe. + /* Indention error. + * Optionally allow fd_buffered_io=1 to be enabled for people + * who know what they are doing w/o O_DSYNC. + */ This comment doesn't explain at all what's going on here. It should be something like /* * Unsafe mode allows disabling data integrity by not forcing * data out to disk in writethrough cache mode. Only to be used * for benchmark cheating or similar purposes. */ #define FBDF_HAS_PATH0x01 #define FBDF_HAS_SIZE0x02 +#define FDBD_USE_BUFFERED_IO 0x04 This should be named BDBD_UNSAFE -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
From: Nicholas Bellinger This patch re-adds the ability to optionally run in buffered FILEIO mode (eg: w/o O_DSYNC) for device backends in order to once again use the Linux buffered cache as a write-back storage mechanism. This logic was originally dropped with mainline v3.5-rc commit: commit a4dff3043c231d57f982af635c9d2192ee40e5ae Author: Nicholas Bellinger Date: Wed May 30 16:25:41 2012 -0700 target/file: Use O_DSYNC by default for FILEIO backends This difference with this patch is that fd_create_virtdevice() now forces the explicit setting of emulate_write_cache=1 when buffered FILEIO operation has been enabled. Reported-by: Ferry Cc: Christoph Hellwig Cc: Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_file.c | 36 +--- drivers/target/target_core_file.h |1 + 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 925..e79b7b9 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -125,6 +125,14 @@ static struct se_device *fd_create_virtdevice( * of pure timestamp updates. */ flags = O_RDWR | O_CREAT | O_LARGEFILE | O_DSYNC; +/* +* Optionally allow fd_buffered_io=1 to be enabled for people +* who know what they are doing w/o O_DSYNC. +*/ + if (fd_dev->fbd_flags & FDBD_USE_BUFFERED_IO) { + pr_debug("FILEIO: Disabling O_DSYNC, using buffered FILEIO\n"); + flags &= ~O_DSYNC; + } file = filp_open(fd_dev->fd_dev_name, flags, 0600); if (IS_ERR(file)) { @@ -188,6 +196,12 @@ static struct se_device *fd_create_virtdevice( if (!dev) goto fail; + if (fd_dev->fbd_flags & FDBD_USE_BUFFERED_IO) { + pr_debug("FILEIO: Forcing setting of emulate_write_cache=1" + " with FDBD_USE_BUFFERED_IO\n"); + dev->se_sub_dev->se_dev_attrib.emulate_write_cache = 1; + } + fd_dev->fd_dev_id = fd_host->fd_host_dev_id_count++; fd_dev->fd_queue_depth = dev->queue_depth; @@ -407,6 +421,7 @@ enum { static match_table_t tokens = { {Opt_fd_dev_name, "fd_dev_name=%s"}, {Opt_fd_dev_size, "fd_dev_size=%s"}, + {Opt_fd_buffered_io, "fd_buffered_io=%d"}, {Opt_err, NULL} }; @@ -418,7 +433,7 @@ static ssize_t fd_set_configfs_dev_params( struct fd_dev *fd_dev = se_dev->se_dev_su_ptr; char *orig, *ptr, *arg_p, *opts; substring_t args[MAX_OPT_ARGS]; - int ret = 0, token; + int ret = 0, arg, token; opts = kstrdup(page, GFP_KERNEL); if (!opts) @@ -459,6 +474,19 @@ static ssize_t fd_set_configfs_dev_params( " bytes\n", fd_dev->fd_dev_size); fd_dev->fbd_flags |= FBDF_HAS_SIZE; break; + case Opt_fd_buffered_io: + match_int(args, ); + if (arg != 1) { + pr_err("bogus fd_buffered_io=%d value\n", arg); + ret = -EINVAL; + goto out; + } + + pr_debug("FILEIO: Using buffered I/O" + " operations for struct fd_dev\n"); + + fd_dev->fbd_flags |= FDBD_USE_BUFFERED_IO; + break; default: break; } @@ -490,8 +518,10 @@ static ssize_t fd_show_configfs_dev_params( ssize_t bl = 0; bl = sprintf(b + bl, "TCM FILEIO ID: %u", fd_dev->fd_dev_id); - bl += sprintf(b + bl, "File: %s Size: %llu Mode: O_DSYNC\n", - fd_dev->fd_dev_name, fd_dev->fd_dev_size); + bl += sprintf(b + bl, "File: %s Size: %llu Mode: %s\n", + fd_dev->fd_dev_name, fd_dev->fd_dev_size, + (fd_dev->fbd_flags & FDBD_USE_BUFFERED_IO) ? + "Buffered" : "O_DSYNC"); return bl; } diff --git a/drivers/target/target_core_file.h b/drivers/target/target_core_file.h index 70ce7fd..fbd59ef 100644 --- a/drivers/target/target_core_file.h +++ b/drivers/target/target_core_file.h @@ -14,6 +14,7 @@ #define FBDF_HAS_PATH 0x01 #define FBDF_HAS_SIZE 0x02 +#define FDBD_USE_BUFFERED_IO 0x04 struct fd_dev { u32 fbd_flags; -- 1.7.2.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/6] target/file: Re-enable optional fd_buffered_io=1 operation
From: Nicholas Bellinger n...@linux-iscsi.org This patch re-adds the ability to optionally run in buffered FILEIO mode (eg: w/o O_DSYNC) for device backends in order to once again use the Linux buffered cache as a write-back storage mechanism. This logic was originally dropped with mainline v3.5-rc commit: commit a4dff3043c231d57f982af635c9d2192ee40e5ae Author: Nicholas Bellinger n...@linux-iscsi.org Date: Wed May 30 16:25:41 2012 -0700 target/file: Use O_DSYNC by default for FILEIO backends This difference with this patch is that fd_create_virtdevice() now forces the explicit setting of emulate_write_cache=1 when buffered FILEIO operation has been enabled. Reported-by: Ferry iscsi...@bananateam.nl Cc: Christoph Hellwig h...@lst.de Cc: sta...@vger.kernel.org Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org --- drivers/target/target_core_file.c | 36 +--- drivers/target/target_core_file.h |1 + 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 925..e79b7b9 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -125,6 +125,14 @@ static struct se_device *fd_create_virtdevice( * of pure timestamp updates. */ flags = O_RDWR | O_CREAT | O_LARGEFILE | O_DSYNC; +/* +* Optionally allow fd_buffered_io=1 to be enabled for people +* who know what they are doing w/o O_DSYNC. +*/ + if (fd_dev-fbd_flags FDBD_USE_BUFFERED_IO) { + pr_debug(FILEIO: Disabling O_DSYNC, using buffered FILEIO\n); + flags = ~O_DSYNC; + } file = filp_open(fd_dev-fd_dev_name, flags, 0600); if (IS_ERR(file)) { @@ -188,6 +196,12 @@ static struct se_device *fd_create_virtdevice( if (!dev) goto fail; + if (fd_dev-fbd_flags FDBD_USE_BUFFERED_IO) { + pr_debug(FILEIO: Forcing setting of emulate_write_cache=1 +with FDBD_USE_BUFFERED_IO\n); + dev-se_sub_dev-se_dev_attrib.emulate_write_cache = 1; + } + fd_dev-fd_dev_id = fd_host-fd_host_dev_id_count++; fd_dev-fd_queue_depth = dev-queue_depth; @@ -407,6 +421,7 @@ enum { static match_table_t tokens = { {Opt_fd_dev_name, fd_dev_name=%s}, {Opt_fd_dev_size, fd_dev_size=%s}, + {Opt_fd_buffered_io, fd_buffered_io=%d}, {Opt_err, NULL} }; @@ -418,7 +433,7 @@ static ssize_t fd_set_configfs_dev_params( struct fd_dev *fd_dev = se_dev-se_dev_su_ptr; char *orig, *ptr, *arg_p, *opts; substring_t args[MAX_OPT_ARGS]; - int ret = 0, token; + int ret = 0, arg, token; opts = kstrdup(page, GFP_KERNEL); if (!opts) @@ -459,6 +474,19 @@ static ssize_t fd_set_configfs_dev_params( bytes\n, fd_dev-fd_dev_size); fd_dev-fbd_flags |= FBDF_HAS_SIZE; break; + case Opt_fd_buffered_io: + match_int(args, arg); + if (arg != 1) { + pr_err(bogus fd_buffered_io=%d value\n, arg); + ret = -EINVAL; + goto out; + } + + pr_debug(FILEIO: Using buffered I/O +operations for struct fd_dev\n); + + fd_dev-fbd_flags |= FDBD_USE_BUFFERED_IO; + break; default: break; } @@ -490,8 +518,10 @@ static ssize_t fd_show_configfs_dev_params( ssize_t bl = 0; bl = sprintf(b + bl, TCM FILEIO ID: %u, fd_dev-fd_dev_id); - bl += sprintf(b + bl, File: %s Size: %llu Mode: O_DSYNC\n, - fd_dev-fd_dev_name, fd_dev-fd_dev_size); + bl += sprintf(b + bl, File: %s Size: %llu Mode: %s\n, + fd_dev-fd_dev_name, fd_dev-fd_dev_size, + (fd_dev-fbd_flags FDBD_USE_BUFFERED_IO) ? + Buffered : O_DSYNC); return bl; } diff --git a/drivers/target/target_core_file.h b/drivers/target/target_core_file.h index 70ce7fd..fbd59ef 100644 --- a/drivers/target/target_core_file.h +++ b/drivers/target/target_core_file.h @@ -14,6 +14,7 @@ #define FBDF_HAS_PATH 0x01 #define FBDF_HAS_SIZE 0x02 +#define FDBD_USE_BUFFERED_IO 0x04 struct fd_dev { u32 fbd_flags; -- 1.7.2.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/