* [PATCH] target: For iblock at default writecache enable. @ 2013-01-25 1:58 majianpeng 2013-01-29 19:03 ` Nicholas A. Bellinger 0 siblings, 1 reply; 6+ messages in thread From: majianpeng @ 2013-01-25 1:58 UTC (permalink / raw) To: nab; +Cc: linux-scsi, target-devel Like the harddisk, at default write cache is enable. Signed-off-by: Jianpeng Ma <majianpeng@gmail.com> --- drivers/target/target_core_iblock.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c index b526d23..69c2f2d 100644 --- a/drivers/target/target_core_iblock.c +++ b/drivers/target/target_core_iblock.c @@ -125,6 +125,9 @@ static int iblock_configure_device(struct se_device *dev) dev->dev_attrib.hw_max_sectors = UINT_MAX; dev->dev_attrib.hw_queue_depth = q->nr_requests; + /*Default enable write cache*/ + dev->dev_attrib.emulate_write_cache = 1; + /* * Check if the underlying struct block_device request_queue supports * the QUEUE_FLAG_DISCARD bit for UNMAP/WRITE_SAME in SCSI + TRIM -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] target: For iblock at default writecache enable. 2013-01-25 1:58 [PATCH] target: For iblock at default writecache enable majianpeng @ 2013-01-29 19:03 ` Nicholas A. Bellinger 2013-01-29 23:04 ` Andy Grover 0 siblings, 1 reply; 6+ messages in thread From: Nicholas A. Bellinger @ 2013-01-29 19:03 UTC (permalink / raw) To: majianpeng; +Cc: linux-scsi, target-devel Hi majianpeng, On Fri, 2013-01-25 at 09:58 +0800, majianpeng wrote: > Like the harddisk, at default write cache is enable. > > Signed-off-by: Jianpeng Ma <majianpeng@gmail.com> > --- > drivers/target/target_core_iblock.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c > index b526d23..69c2f2d 100644 > --- a/drivers/target/target_core_iblock.c > +++ b/drivers/target/target_core_iblock.c > @@ -125,6 +125,9 @@ static int iblock_configure_device(struct se_device *dev) > dev->dev_attrib.hw_max_sectors = UINT_MAX; > dev->dev_attrib.hw_queue_depth = q->nr_requests; > > + /*Default enable write cache*/ > + dev->dev_attrib.emulate_write_cache = 1; > + So enabling emulate_write_cache=1 in the case when the underlying device has not enabled it is incorrect. I'd like to enable this bit when we know the underlying device has WCE=1 set, but currently there is not a way to determine this (generically) from struct block_device. So NACK for applying this until there is a method to determine what the hardware below is doing. --nab > /* > * Check if the underlying struct block_device request_queue supports > * the QUEUE_FLAG_DISCARD bit for UNMAP/WRITE_SAME in SCSI + TRIM > -- > 1.7.9.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target: For iblock at default writecache enable. 2013-01-29 19:03 ` Nicholas A. Bellinger @ 2013-01-29 23:04 ` Andy Grover 2013-01-29 23:14 ` Nicholas A. Bellinger 0 siblings, 1 reply; 6+ messages in thread From: Andy Grover @ 2013-01-29 23:04 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: majianpeng, linux-scsi, target-devel On 01/29/2013 11:03 AM, Nicholas A. Bellinger wrote: > So enabling emulate_write_cache=1 in the case when the underlying device > has not enabled it is incorrect. > > I'd like to enable this bit when we know the underlying device has WCE=1 > set, but currently there is not a way to determine this (generically) > from struct block_device. > > So NACK for applying this until there is a method to determine what the > hardware below is doing. This should be possible from userspace though. I'm planning on looking up underlying scsi device(s?) using libblkid, and then query the sense data using libsgutils when adding a block backstore in targetcli. It's kind of a hassle, but isn't it a huge performance win if we can enable this? Regards -- Andy ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target: For iblock at default writecache enable. 2013-01-29 23:04 ` Andy Grover @ 2013-01-29 23:14 ` Nicholas A. Bellinger 2013-01-30 1:19 ` majianpeng 0 siblings, 1 reply; 6+ messages in thread From: Nicholas A. Bellinger @ 2013-01-29 23:14 UTC (permalink / raw) To: Andy Grover; +Cc: majianpeng, linux-scsi, target-devel On Tue, 2013-01-29 at 15:04 -0800, Andy Grover wrote: > On 01/29/2013 11:03 AM, Nicholas A. Bellinger wrote: > > > So enabling emulate_write_cache=1 in the case when the underlying device > > has not enabled it is incorrect. > > > > I'd like to enable this bit when we know the underlying device has WCE=1 > > set, but currently there is not a way to determine this (generically) > > from struct block_device. > > > > So NACK for applying this until there is a method to determine what the > > hardware below is doing. > > This should be possible from userspace though. I'm planning on looking > up underlying scsi device(s?) using libblkid, and then query the sense > data using libsgutils when adding a block backstore in targetcli. > Querying the mode pages from userspace would work for the SCSI backstore case, but certainly not for raw block devices. I'd still like to see this exposed to the block layer somehow, so that the setting can be automatically determined by TCM once it's known if the underlying HW has enabled write caching. > It's kind of a hassle, but isn't it a huge performance win if we can > enable this? > Most certainly, but the danger is reporting WCE=1 (by default in all cases) from TCM to the initiator when the underlying drives do not have caching enabled. Note that every spinning media device that I've ever seen disables WCE by default from the factory. --nab ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] target: For iblock at default writecache enable. 2013-01-29 23:14 ` Nicholas A. Bellinger @ 2013-01-30 1:19 ` majianpeng 2013-01-30 7:29 ` Nicholas A. Bellinger 0 siblings, 1 reply; 6+ messages in thread From: majianpeng @ 2013-01-30 1:19 UTC (permalink / raw) To: nab, Andy Grover; +Cc: linux-scsi, target-devel >On Tue, 2013-01-29 at 15:04 -0800, Andy Grover wrote: >> On 01/29/2013 11:03 AM, Nicholas A. Bellinger wrote: >> >> > So enabling emulate_write_cache=1 in the case when the underlying device >> > has not enabled it is incorrect. >> > >> > I'd like to enable this bit when we know the underlying device has WCE=1 >> > set, but currently there is not a way to determine this (generically) >> > from struct block_device. >> > >> > So NACK for applying this until there is a method to determine what the >> > hardware below is doing. >> >> This should be possible from userspace though. I'm planning on looking >> up underlying scsi device(s?) using libblkid, and then query the sense >> data using libsgutils when adding a block backstore in targetcli. >> > >Querying the mode pages from userspace would work for the SCSI backstore >case, but certainly not for raw block devices. > >I'd still like to see this exposed to the block layer somehow, so that >the setting can be automatically determined by TCM once it's known if >the underlying HW has enabled write caching. > >> It's kind of a hassle, but isn't it a huge performance win if we can >> enable this? >> > >Most certainly, but the danger is reporting WCE=1 (by default in all >cases) from TCM to the initiator when the underlying drives do not have >caching enabled. Note that every spinning media device that I've ever >seen disables WCE by default from the factory. Sorry, what's the danger?Can you explain the details? And for most sata hdd the WCE is enable by default. IMHO, for hard-raid the WCE will be disable, it used the cache of hardraid-card. In func sd_revalidate_disk: > /* > * We now have all cache related info, determine how we deal > * with flush requests. > */ > if (sdkp->WCE) { > flush |= REQ_FLUSH; > if (sdkp->DPOFUA) > flush |= REQ_FUA; > } > blk_queue_flush(sdkp->disk->queue, flush); We can use queue->flush_flags. But in func generic_make_request_checks: >/* > * Filter flush bio's early so that make_request based > * drivers without flush support don't have to worry > * about them. > */ > if ((bio->bi_rw & (REQ_FLUSH | REQ_FUA)) && !q->flush_flags) { > bio->bi_rw &= ~(REQ_FLUSH | REQ_FUA); So if uderlying device don't support WCE, it can remove REQ_FUA|REQ_FLUSH. I think enable writecache by default is ok. Jianpeng Thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] target: For iblock at default writecache enable. 2013-01-30 1:19 ` majianpeng @ 2013-01-30 7:29 ` Nicholas A. Bellinger 0 siblings, 0 replies; 6+ messages in thread From: Nicholas A. Bellinger @ 2013-01-30 7:29 UTC (permalink / raw) To: majianpeng Cc: Andy Grover, linux-scsi, target-devel, Christoph Hellwig, Jens Axboe, James Bottomley Hi majianpeng, On Wed, 2013-01-30 at 09:19 +0800, majianpeng wrote: > >On Tue, 2013-01-29 at 15:04 -0800, Andy Grover wrote: > >> On 01/29/2013 11:03 AM, Nicholas A. Bellinger wrote: > >> > >> > So enabling emulate_write_cache=1 in the case when the underlying device > >> > has not enabled it is incorrect. > >> > > >> > I'd like to enable this bit when we know the underlying device has WCE=1 > >> > set, but currently there is not a way to determine this (generically) > >> > from struct block_device. > >> > > >> > So NACK for applying this until there is a method to determine what the > >> > hardware below is doing. > >> > >> This should be possible from userspace though. I'm planning on looking > >> up underlying scsi device(s?) using libblkid, and then query the sense > >> data using libsgutils when adding a block backstore in targetcli. > >> > > > >Querying the mode pages from userspace would work for the SCSI backstore > >case, but certainly not for raw block devices. > > > >I'd still like to see this exposed to the block layer somehow, so that > >the setting can be automatically determined by TCM once it's known if > >the underlying HW has enabled write caching. > > > >> It's kind of a hassle, but isn't it a huge performance win if we can > >> enable this? > >> > > > >Most certainly, but the danger is reporting WCE=1 (by default in all > >cases) from TCM to the initiator when the underlying drives do not have > >caching enabled. Note that every spinning media device that I've ever > >seen disables WCE by default from the factory. > > Sorry, what's the danger?Can you explain the details? Current IBLOCK code will always use WRITE_FUA with emulate_write_cache=0 default setting. It attempts to be safe and invoke write through schematics with WRITE_FUA when emulate_write_cache=0 is present. > And for most sata hdd the WCE is enable by default. Spinning media SAS drives from the factory still this disable per default to avoid silent data loss in the event of a hard power failure. > IMHO, for hard-raid the WCE will be disable, it used the cache of hardraid-card. > > In func sd_revalidate_disk: > > /* > > * We now have all cache related info, determine how we deal > > * with flush requests. > > */ > > if (sdkp->WCE) { > > flush |= REQ_FLUSH; > > if (sdkp->DPOFUA) > > flush |= REQ_FUA; > > } > > > blk_queue_flush(sdkp->disk->queue, flush); > We can use queue->flush_flags. Good point. These are request queue hints that IBLOCK could be taking advantage of here. > But in func generic_make_request_checks: > >/* > > * Filter flush bio's early so that make_request based > > * drivers without flush support don't have to worry > > * about them. > > */ > > if ((bio->bi_rw & (REQ_FLUSH | REQ_FUA)) && !q->flush_flags) { > > bio->bi_rw &= ~(REQ_FLUSH | REQ_FUA); > So if uderlying device don't support WCE, it can remove REQ_FUA|REQ_FLUSH. > I think enable writecache by default is ok. > Blindly enabling WriteCacheEnabled=1 in all cases for IBLOCK backends to the SCSI initiator port, and ignoring what the underlying hardware is doing is not correct either. So, I've sent out a patch proposing a method for IBLOCK to take advantage of REQ_FLUSH for WCE=1. I'm not crazy about the extra pointer dereferences in the iblock_execute_rw() data I/O path, but it's likely the most sane way to access to request_queue->flush_flags minus adding a target callback to update emulate_write_cache in the event of a blk_queue_flush() call, considering this value may change from below target/iblock. (hch, axboe + jejb CC'ed) --nab ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-30 7:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-25 1:58 [PATCH] target: For iblock at default writecache enable majianpeng 2013-01-29 19:03 ` Nicholas A. Bellinger 2013-01-29 23:04 ` Andy Grover 2013-01-29 23:14 ` Nicholas A. Bellinger 2013-01-30 1:19 ` majianpeng 2013-01-30 7:29 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).