On Sun, Sep 30, 2012 at 05:58:11AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> 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_PATH 0x01
> #define FBDF_HAS_SIZE 0x02
> +#define FDBD_USE_BUFFERED_IO 0x04
This should be named BDBD_UNSAFE
Christoph Hellwig, on 10/01/2012 04:46 AM wrote:
> On Sun, Sep 30, 2012 at 05:58:11AM +0000, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger<[email protected]>
>>
>> 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
On Mon, 2012-10-01 at 04:46 -0400, Christoph Hellwig wrote:
> On Sun, Sep 30, 2012 at 05:58:11AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[email protected]>
> >
> > 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..?
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?
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