* [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support @ 2010-09-27 22:51 Nicholas A. Bellinger 2010-09-28 6:46 ` Boaz Harrosh 2010-09-28 21:06 ` Rolf Eike Beer 0 siblings, 2 replies; 6+ messages in thread From: Nicholas A. Bellinger @ 2010-09-27 22:51 UTC (permalink / raw) To: linux-scsi, linux-kernel Cc: Christoph Hellwig, Martin K. Petersen, Douglas Gilbert, Jens Axboe, FUJITA Tomonori, Mike Christie, Hannes Reinecke, James Bottomley, Konrad Rzeszutek Wilk, Boaz Harrosh, Richard Sharpe, Nicholas Bellinger From: Nicholas Bellinger <nab@linux-iscsi.org> This patch adds UNMAP emulation support using Block layer DISCARD in fd_emulate_scsi_cdb() -> transport_generic_unmap(). This includes the use of blk_queue_discard() in fd_create_virtdevice() to determine when to set DEV_ATTRIB(dev)->emulate_tpe=1 to signal to TCM Core to perform the necessary control CDB emulation for TPE=1 / UNMAP. Note this patch igrab() + iput() when accessing struct block_device via I_BDEV(file->f_mapping->host) that is required for transport_generic_unmap() within fd_emulate_scsi_cdb(). Also note that when (S_ISBLK(inode->i_mode) is NULL, the TPE=1 and UNMAP emulation are disabled for non BD struct file I/O. Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org> --- drivers/target/target_core_file.c | 53 ++++++++++++++++++++++++++++++++++++- 1 files changed, 52 insertions(+), 1 deletions(-) diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 93d3f63..3b91556 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -235,6 +235,17 @@ static struct se_device *fd_create_virtdevice( fd_dev->fd_dev_id = fd_host->fd_host_dev_id_count++; fd_dev->fd_queue_depth = dev->queue_depth; + /* + * Check for QUEUE_FLAG_DISCARD and enable TCM TPE emulation + * if present for struct block_device backend + */ + if (S_ISBLK(inode->i_mode)) { + if (blk_queue_discard(bdev_get_queue(inode->i_bdev))) { + DEV_ATTRIB(dev)->emulate_tpe = 1; + printk(KERN_INFO "FILEIO: Enabling BLOCK Discard" + " and TPE=1 emulation\n"); + } + } printk(KERN_INFO "CORE_FILE[%u] - Added TCM FILEIO Device ID: %u at %s," " %llu total bytes\n", fd_host->fd_host_id, fd_dev->fd_dev_id, @@ -388,9 +399,13 @@ static int fd_emulate_read_cap16(struct se_task *task) */ static int fd_emulate_scsi_cdb(struct se_task *task) { - int ret; struct se_cmd *cmd = TASK_CMD(task); + struct fd_dev *fd_dev = (struct fd_dev *) task->se_dev->dev_ptr; struct fd_request *fd_req = (struct fd_request *) task->transport_req; + struct file *f = fd_dev->fd_file; + struct inode *i; + struct block_device *bd; + int ret; switch (fd_req->fd_scsi_cdb[0]) { case INQUIRY: @@ -433,6 +448,42 @@ static int fd_emulate_scsi_cdb(struct se_task *task) if (ret < 0) return ret; break; + case UNMAP: + i = igrab(f->f_mapping->host); + if (!(i)) { + printk(KERN_ERR "FILEIO: Unable to locate inode for" + " backend for UNMAP\n"); + return PYX_TRANSPORT_LU_COMM_FAILURE; + } + /* + * Currently for struct file w/o a struct block_device + * backend we return a success.. + */ + if (!(S_ISBLK(i->i_mode))) { + printk(KERN_WARNING "Ignoring UNMAP for non BD" + " backend for struct file\n"); + iput(i); + return PYX_TRANSPORT_LU_COMM_FAILURE; + } + bd = I_BDEV(f->f_mapping->host); + if (!(bd)) { + printk(KERN_ERR "FILEIO: Unable to locate struct" + " block_device for UNMAP\n"); + iput(i); + return PYX_TRANSPORT_LU_COMM_FAILURE; + } + /* + * Now call the transport_generic_unmap() -> blkdev_issue_discard() + * wrapper to translate SCSI UNMAP into Linux/BLOCK discards on + * LBA+Range descriptors in the UNMAP write paylaod. + */ + ret = transport_generic_unmap(cmd, bd); + if (ret < 0) { + iput(i); + return ret; + } + iput(i); + break; case ALLOW_MEDIUM_REMOVAL: case ERASE: case REZERO_UNIT: -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support 2010-09-27 22:51 [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support Nicholas A. Bellinger @ 2010-09-28 6:46 ` Boaz Harrosh 2010-09-29 8:08 ` Nicholas A. Bellinger 2010-09-28 21:06 ` Rolf Eike Beer 1 sibling, 1 reply; 6+ messages in thread From: Boaz Harrosh @ 2010-09-28 6:46 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: linux-scsi, linux-kernel, Christoph Hellwig, Martin K. Petersen, Douglas Gilbert, Jens Axboe, FUJITA Tomonori, Mike Christie, Hannes Reinecke, James Bottomley, Konrad Rzeszutek Wilk, Richard Sharpe On 09/28/2010 12:51 AM, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger <nab@linux-iscsi.org> > > This patch adds UNMAP emulation support using Block layer DISCARD in fd_emulate_scsi_cdb() > -> transport_generic_unmap(). This includes the use of blk_queue_discard() in > fd_create_virtdevice() to determine when to set DEV_ATTRIB(dev)->emulate_tpe=1 to > signal to TCM Core to perform the necessary control CDB emulation for TPE=1 / UNMAP. > > Note this patch igrab() + iput() when accessing struct block_device via > I_BDEV(file->f_mapping->host) that is required for transport_generic_unmap() > within fd_emulate_scsi_cdb(). > > Also note that when (S_ISBLK(inode->i_mode) is NULL, the TPE=1 and UNMAP emulation > are disabled for non BD struct file I/O. > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org> > --- > drivers/target/target_core_file.c | 53 ++++++++++++++++++++++++++++++++++++- > 1 files changed, 52 insertions(+), 1 deletions(-) > > diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c > index 93d3f63..3b91556 100644 > --- a/drivers/target/target_core_file.c > +++ b/drivers/target/target_core_file.c > @@ -235,6 +235,17 @@ static struct se_device *fd_create_virtdevice( > > fd_dev->fd_dev_id = fd_host->fd_host_dev_id_count++; > fd_dev->fd_queue_depth = dev->queue_depth; > + /* > + * Check for QUEUE_FLAG_DISCARD and enable TCM TPE emulation > + * if present for struct block_device backend > + */ > + if (S_ISBLK(inode->i_mode)) { > + if (blk_queue_discard(bdev_get_queue(inode->i_bdev))) { > + DEV_ATTRIB(dev)->emulate_tpe = 1; > + printk(KERN_INFO "FILEIO: Enabling BLOCK Discard" > + " and TPE=1 emulation\n"); > + } > + } > > printk(KERN_INFO "CORE_FILE[%u] - Added TCM FILEIO Device ID: %u at %s," > " %llu total bytes\n", fd_host->fd_host_id, fd_dev->fd_dev_id, > @@ -388,9 +399,13 @@ static int fd_emulate_read_cap16(struct se_task *task) > */ > static int fd_emulate_scsi_cdb(struct se_task *task) > { > - int ret; > struct se_cmd *cmd = TASK_CMD(task); > + struct fd_dev *fd_dev = (struct fd_dev *) task->se_dev->dev_ptr; No need to typecast a void pointer > struct fd_request *fd_req = (struct fd_request *) task->transport_req; > + struct file *f = fd_dev->fd_file; > + struct inode *i; > + struct block_device *bd; > + int ret; > > switch (fd_req->fd_scsi_cdb[0]) { > case INQUIRY: > @@ -433,6 +448,42 @@ static int fd_emulate_scsi_cdb(struct se_task *task) > if (ret < 0) > return ret; > break; > + case UNMAP: > + i = igrab(f->f_mapping->host); > + if (!(i)) { You must have a reference on your fileio backend file, right? This could be a BUG_ON() > + printk(KERN_ERR "FILEIO: Unable to locate inode for" > + " backend for UNMAP\n"); > + return PYX_TRANSPORT_LU_COMM_FAILURE; > + } > + /* > + * Currently for struct file w/o a struct block_device > + * backend we return a success.. > + */ > + if (!(S_ISBLK(i->i_mode))) { I did not see in the first patch that you do this check when actually sending the VPD page. I only saw that it is enabled if set from configfs. (Sorry for the noise if I missed it). So you might want to call a target specific function when sending the VPD page. This way this will never happen right? > + printk(KERN_WARNING "Ignoring UNMAP for non BD" > + " backend for struct file\n"); TODO: Lots of extent based filesystems could enjoy if you punch an hole in the file. (Which will also send a proper discard). So how to punch an hole in a file via vfs? > + iput(i); > + return PYX_TRANSPORT_LU_COMM_FAILURE; > + } > + bd = I_BDEV(f->f_mapping->host); > + if (!(bd)) { > + printk(KERN_ERR "FILEIO: Unable to locate struct" > + " block_device for UNMAP\n"); > + iput(i); > + return PYX_TRANSPORT_LU_COMM_FAILURE; > + } > + /* > + * Now call the transport_generic_unmap() -> blkdev_issue_discard() > + * wrapper to translate SCSI UNMAP into Linux/BLOCK discards on > + * LBA+Range descriptors in the UNMAP write paylaod. > + */ > + ret = transport_generic_unmap(cmd, bd); > + if (ret < 0) { > + iput(i); > + return ret; > + } > + iput(i); > + break; > case ALLOW_MEDIUM_REMOVAL: > case ERASE: > case REZERO_UNIT: Boaz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support 2010-09-28 6:46 ` Boaz Harrosh @ 2010-09-29 8:08 ` Nicholas A. Bellinger 2010-09-29 8:57 ` Boaz Harrosh 0 siblings, 1 reply; 6+ messages in thread From: Nicholas A. Bellinger @ 2010-09-29 8:08 UTC (permalink / raw) To: Boaz Harrosh Cc: linux-scsi, linux-kernel, Christoph Hellwig, Martin K. Petersen, Douglas Gilbert, Jens Axboe, FUJITA Tomonori, Mike Christie, Hannes Reinecke, James Bottomley, Konrad Rzeszutek Wilk, Richard Sharpe On Tue, 2010-09-28 at 08:46 +0200, Boaz Harrosh wrote: > On 09/28/2010 12:51 AM, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > > > This patch adds UNMAP emulation support using Block layer DISCARD in fd_emulate_scsi_cdb() > > -> transport_generic_unmap(). This includes the use of blk_queue_discard() in > > fd_create_virtdevice() to determine when to set DEV_ATTRIB(dev)->emulate_tpe=1 to > > signal to TCM Core to perform the necessary control CDB emulation for TPE=1 / UNMAP. > > > > Note this patch igrab() + iput() when accessing struct block_device via > > I_BDEV(file->f_mapping->host) that is required for transport_generic_unmap() > > within fd_emulate_scsi_cdb(). > > > > Also note that when (S_ISBLK(inode->i_mode) is NULL, the TPE=1 and UNMAP emulation > > are disabled for non BD struct file I/O. > > > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org> > > --- > > drivers/target/target_core_file.c | 53 ++++++++++++++++++++++++++++++++++++- > > 1 files changed, 52 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c > > index 93d3f63..3b91556 100644 > > --- a/drivers/target/target_core_file.c > > +++ b/drivers/target/target_core_file.c > > @@ -235,6 +235,17 @@ static struct se_device *fd_create_virtdevice( > > > > fd_dev->fd_dev_id = fd_host->fd_host_dev_id_count++; > > fd_dev->fd_queue_depth = dev->queue_depth; > > + /* > > + * Check for QUEUE_FLAG_DISCARD and enable TCM TPE emulation > > + * if present for struct block_device backend > > + */ > > + if (S_ISBLK(inode->i_mode)) { > > + if (blk_queue_discard(bdev_get_queue(inode->i_bdev))) { > > + DEV_ATTRIB(dev)->emulate_tpe = 1; > > + printk(KERN_INFO "FILEIO: Enabling BLOCK Discard" > > + " and TPE=1 emulation\n"); > > + } > > + } > > > > printk(KERN_INFO "CORE_FILE[%u] - Added TCM FILEIO Device ID: %u at %s," > > " %llu total bytes\n", fd_host->fd_host_id, fd_dev->fd_dev_id, > > @@ -388,9 +399,13 @@ static int fd_emulate_read_cap16(struct se_task *task) > > */ > > static int fd_emulate_scsi_cdb(struct se_task *task) > > { > > - int ret; > > struct se_cmd *cmd = TASK_CMD(task); > > + struct fd_dev *fd_dev = (struct fd_dev *) task->se_dev->dev_ptr; > > No need to typecast a void pointer Whoops, this was a cut and paste but these unnecessary typecasts for 'struct sub_dev' and 'struct sub_task' have been around forever and can go. Removing these in target_core_file.c, target_core_iblock, target_core_pscsi.c target_core_rd.c, and target_core_stgt.c now. > > > struct fd_request *fd_req = (struct fd_request *) task->transport_req; > > + struct file *f = fd_dev->fd_file; > > + struct inode *i; > > + struct block_device *bd; > > + int ret; > > > > switch (fd_req->fd_scsi_cdb[0]) { > > case INQUIRY: > > @@ -433,6 +448,42 @@ static int fd_emulate_scsi_cdb(struct se_task *task) > > if (ret < 0) > > return ret; > > break; > > + case UNMAP: > > + i = igrab(f->f_mapping->host); > > + if (!(i)) { > > You must have a reference on your fileio backend file, right? > This could be a BUG_ON() Yes, this was an extra bit of paranoia as we always expect to have a valid struct file here. > > > + printk(KERN_ERR "FILEIO: Unable to locate inode for" > > + " backend for UNMAP\n"); > > + return PYX_TRANSPORT_LU_COMM_FAILURE; > > + } > > + /* > > + * Currently for struct file w/o a struct block_device > > + * backend we return a success.. > > + */ > > + if (!(S_ISBLK(i->i_mode))) { > > I did not see in the first patch that you do this check when actually sending > the VPD page. I only saw that it is enabled if set from configfs. (Sorry for > the noise if I missed it). So you might want to call a target specific function > when sending the VPD page. This way this will never happen right? That is correct, once we determine that fd_create_virtdevice() -> blk_queue_discard() is going to set DEV_ATTRIB(dev)->emulate_tpe=1, this is what will signal TPE=1 in SA READ_CAPACITY_16 and other locations in Patch 1 for the emulated case with TCM/IBLOCK and TCM/FILEIO. > > > + printk(KERN_WARNING "Ignoring UNMAP for non BD" > > + " backend for struct file\n"); > > TODO: Lots of extent based filesystems could enjoy if you punch an hole > in the file. (Which will also send a proper discard). > So how to punch an hole in a file via vfs? Good question, as all of the current block discard support assumes a struct block_device * pointer currently. I know that hch has mentioned that accesing a struct block_device from struct file is very dangerous, but I am assuming for the TCM/FILEIO case that this code will be protected by igrab() and iput(). hch and jaxboe, any comments here..? Thanks! --nab > > > + iput(i); > > + return PYX_TRANSPORT_LU_COMM_FAILURE; > > + } > > + bd = I_BDEV(f->f_mapping->host); > > + if (!(bd)) { > > + printk(KERN_ERR "FILEIO: Unable to locate struct" > > + " block_device for UNMAP\n"); > > + iput(i); > > + return PYX_TRANSPORT_LU_COMM_FAILURE; > > + } > > + /* > > + * Now call the transport_generic_unmap() -> blkdev_issue_discard() > > + * wrapper to translate SCSI UNMAP into Linux/BLOCK discards on > > + * LBA+Range descriptors in the UNMAP write paylaod. > > + */ > > + ret = transport_generic_unmap(cmd, bd); > > + if (ret < 0) { > > + iput(i); > > + return ret; > > + } > > + iput(i); > > + break; > > case ALLOW_MEDIUM_REMOVAL: > > case ERASE: > > case REZERO_UNIT: > > Boaz > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support 2010-09-29 8:08 ` Nicholas A. Bellinger @ 2010-09-29 8:57 ` Boaz Harrosh 2010-09-29 9:08 ` Nicholas A. Bellinger 0 siblings, 1 reply; 6+ messages in thread From: Boaz Harrosh @ 2010-09-29 8:57 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: linux-scsi, linux-kernel, Christoph Hellwig, Martin K. Petersen, Douglas Gilbert, Jens Axboe, FUJITA Tomonori, Mike Christie, Hannes Reinecke, James Bottomley, Konrad Rzeszutek Wilk, Richard Sharpe On 09/29/2010 10:08 AM, Nicholas A. Bellinger wrote: > On Tue, 2010-09-28 at 08:46 +0200, Boaz Harrosh wrote: >>> + printk(KERN_WARNING "Ignoring UNMAP for non BD" >>> + " backend for struct file\n"); >> >> TODO: Lots of extent based filesystems could enjoy if you punch an hole >> in the file. (Which will also send a proper discard). >> So how to punch an hole in a file via vfs? > > Good question, as all of the current block discard support assumes a > struct block_device * pointer currently. > > I know that hch has mentioned that accesing a struct block_device from > struct file is very dangerous, but I am assuming for the TCM/FILEIO case > that this code will be protected by igrab() and iput(). > > hch and jaxboe, any comments here..? > No you did not understand. FILEIO (working on a filesystem's real file), should *never* attempt a discard on the blocks even if it was to read the file's map and figure out these blocks. It should always call a filesystem specific API that puntches-an-hole in the file. Fore stupid filesystem that means fill with zero's. For a smart extent-based file system it means splitting up the extents the range belongs to and freeing up the block that where allocated for that range. But it is an heavy meta-data operation that only the filesystem can do. With really smart FS like xfs the unallocated blocks also get discarded, for when working with SANs with over provisioning. > Thanks! > > --nab Boaz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support 2010-09-29 8:57 ` Boaz Harrosh @ 2010-09-29 9:08 ` Nicholas A. Bellinger 0 siblings, 0 replies; 6+ messages in thread From: Nicholas A. Bellinger @ 2010-09-29 9:08 UTC (permalink / raw) To: Boaz Harrosh Cc: linux-scsi, linux-kernel, Christoph Hellwig, Martin K. Petersen, Douglas Gilbert, Jens Axboe, FUJITA Tomonori, Mike Christie, Hannes Reinecke, James Bottomley, Konrad Rzeszutek Wilk, Richard Sharpe On Wed, 2010-09-29 at 10:57 +0200, Boaz Harrosh wrote: > On 09/29/2010 10:08 AM, Nicholas A. Bellinger wrote: > > On Tue, 2010-09-28 at 08:46 +0200, Boaz Harrosh wrote: > >>> + printk(KERN_WARNING "Ignoring UNMAP for non BD" > >>> + " backend for struct file\n"); > >> > >> TODO: Lots of extent based filesystems could enjoy if you punch an hole > >> in the file. (Which will also send a proper discard). > >> So how to punch an hole in a file via vfs? > > > > Good question, as all of the current block discard support assumes a > > struct block_device * pointer currently. > > > > I know that hch has mentioned that accesing a struct block_device from > > struct file is very dangerous, but I am assuming for the TCM/FILEIO case > > that this code will be protected by igrab() and iput(). > > > > hch and jaxboe, any comments here..? > > > > No you did not understand. FILEIO (working on a filesystem's real file), > should *never* attempt a discard on the blocks even if it was > to read the file's map and figure out these blocks. It should always > call a filesystem specific API that puntches-an-hole in the file. Mmmm, then I will have to defer to the FS folks on this particular item then. > Fore stupid filesystem that means fill with zero's. For a smart > extent-based file system it means splitting up the extents the range > belongs to and freeing up the block that where allocated for that > range. But it is an heavy meta-data operation that only the filesystem > can do. With really smart FS like xfs the unallocated blocks also > get discarded, for when working with SANs with over provisioning. > Ok, that makes sense.. But I guess I am still lost on how this could actually be done for the !(S_ISBLK) struct file case..? Best, --nab ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support 2010-09-27 22:51 [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support Nicholas A. Bellinger 2010-09-28 6:46 ` Boaz Harrosh @ 2010-09-28 21:06 ` Rolf Eike Beer 1 sibling, 0 replies; 6+ messages in thread From: Rolf Eike Beer @ 2010-09-28 21:06 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: linux-scsi, linux-kernel, Christoph Hellwig, Martin K. Petersen, Douglas Gilbert, Jens Axboe, FUJITA Tomonori, Mike Christie, Hannes Reinecke, James Bottomley, Konrad Rzeszutek Wilk, Boaz Harrosh, Richard Sharpe [-- Attachment #1: Type: Text/Plain, Size: 323 bytes --] Nicholas A. Bellinger wrote: > @@ -433,6 +448,42 @@ static int fd_emulate_scsi_cdb(struct se_task *task) > if (ret < 0) > return ret; > break; > + case UNMAP: > + i = igrab(f->f_mapping->host); > + if (!(i)) { ..... > + if (!(S_ISBLK(i->i_mode))) { ..... > + if (!(bd)) { Is this LISP or what? ;) Eike [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-09-29 9:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-27 22:51 [PATCH 3/3] tcm/fileio: Add UNMAP / Block DISCARD support Nicholas A. Bellinger 2010-09-28 6:46 ` Boaz Harrosh 2010-09-29 8:08 ` Nicholas A. Bellinger 2010-09-29 8:57 ` Boaz Harrosh 2010-09-29 9:08 ` Nicholas A. Bellinger 2010-09-28 21:06 ` Rolf Eike Beer
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).