tcm/fileio: Add UNMAP / Block DISCARD support
Subsystems (2)
PATCH 3/3 tcm/fileio: Add UNMAP / Block DISCARD support
From: Nicholas Bellinger <[email protected]>
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.
--- drivers/target/target_core_file.c | 53 ++++++++++++++++++++++++++++++++++++- 1 files changed, 52 insertions(+), 1 deletions(-)
drivers/target/target_core_file.c +52 -1
@@ -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 :
Signature · 1 line
1.5.6.5
Quoted 42 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch adds UNMAP emulation support using Block layer DISCARD in
fd_emulate_scsi_cdb()->transport_generic_unmap(). This includes the use ofblk_queue_discard()infd_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 structblock_deviceviaI_BDEV(file->f_mapping->host)that is required fortransport_generic_unmap()withinfd_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 <[email protected]>--- drivers/target/target_core_file.c | 53 ++++++++++++++++++++++++++++++++++++- 1 files changed, 52 insertions(+), 1 deletions(-)drivers/target/target_core_file.c +11
-0@@ -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,
drivers/target/target_core_file.c static int fd_emulate_read_cap16(struct se_task *task) · line 403
Quoted 6 lines
drivers/target/target_core_file.c +1
-1@@ -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
Quoted 9 lines
drivers/target/target_core_file.c +4
-0struct 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 :
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 453
Quoted 6 lines
drivers/target/target_core_file.c +3
-0@@ -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()
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 462
Quoted 9 lines
drivers/target/target_core_file.c +9
-0+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?
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 464
Quoted 2 lines
drivers/target/target_core_file.c +2
-0+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?
Quoted 15 lines
drivers/target/target_core_file.c +15
-0+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. +*/
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 489
Quoted 10 lines
drivers/target/target_core_file.c +7
-0+ 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
Quoted 40 lines · Boaz Harrosh
Quoted 39 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch adds UNMAP emulation support using Block layer DISCARD in
fd_emulate_scsi_cdb()->transport_generic_unmap(). This includes the use ofblk_queue_discard()infd_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 structblock_deviceviaI_BDEV(file->f_mapping->host)that is required fortransport_generic_unmap()withinfd_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 <[email protected]>--- drivers/target/target_core_file.c | 53 ++++++++++++++++++++++++++++++++++++- 1 files changed, 52 insertions(+), 1 deletions(-)drivers/target/target_core_file.c +11
-0@@ -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" ); + } + }
Quoted 11 lines
drivers/target/target_core_file.c static int fd_emulate_read_cap16(struct se_task *task) · line 403
Quoted 9 lines
drivers/target/target_core_file.c +1
-1printk(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.
Quoted 7 lines
Quoted 7 lines
drivers/target/target_core_file.c +4
-0struct 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;
Quoted 12 lines
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 453
Quoted 9 lines
drivers/target/target_core_file.c +3
-0switch (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.
Quoted 15 lines
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 462
Quoted 10 lines
drivers/target/target_core_file.c +9
-0+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.
Quoted 7 lines
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 464
Quoted 3 lines
drivers/target/target_core_file.c +2
-0+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
Quoted 29 lines
Quoted 16 lines
drivers/target/target_core_file.c +15
-0+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. +*/ drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 489
Quoted 10 lines
drivers/target/target_core_file.c +7
-0+ 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
Quoted 8 lines · Nicholas A. Bellinger
Quoted 6 lines · Boaz Harrosh
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 464
Quoted 2 lines
drivers/target/target_core_file.c +2
-0+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?
Quoted 9 lines
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_devicefrom struct file is very dangerous, but I am assuming for the TCM/FILEIO case that this code will be protected byigrab()andiput().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.
Quoted 3 lines
Thanks!
--nab
Boaz
Quoted 16 lines · Boaz Harrosh
Quoted 15 lines · Nicholas A. Bellinger
Quoted 6 lines · Boaz Harrosh
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 464
Quoted 2 lines
drivers/target/target_core_file.c +2
-0+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_devicefrom struct file is very dangerous, but I am assuming for the TCM/FILEIO case that this code will be protected byigrab()andiput().
Quoted 7 lines
Quoted 2 lines
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.
Quoted 7 lines
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
Quoted 7 lines · Nicholas A. Bellinger
drivers/target/target_core_file.c +3
-0@@ -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)) {
.....
Quoted 1 line
drivers/target/target_core_file.c +1
-0+if (!(S_ISBLK (i->i_mode))) {
.....
drivers/target/target_core_file.c static int fd_emulate_scsi_cdb(struct se_task *task) · line 469
Quoted 1 line
drivers/target/target_core_file.c +1
-0+if (!(bd)) {
Is this LISP or what? ;)
Eike