[PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling

[PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds the following struct se_subsystem_api function pointer
op for UNMAP and WRITE_SAME w/ UNMAP=1 handling mapped to Linux/Block
layer generic Discard to underlying SCSI *_UNMAP and ATA TRIM operation:

       /*
        * Used by virtual subsystem plugins IBLOCK and FILEIO to emulate
        * UNMAP and WRITE_SAME_* w/ UNMAP=1 <-> Linux/Block Discard
        */
       int (*do_discard)(struct se_task *, enum blk_discard_type);

and updates IBLOCK and FILEIO to execute their respective -> Linux/Block
DISCARD ops based on the passed blk_discard_type.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
Reported-by: Christoph Hellwig <hch@lst.de>
---
 drivers/target/target_core_file.c      |   17 +++++++++++++++--
 drivers/target/target_core_iblock.c    |   30 +++++++++++++++++++-----------
 drivers/target/target_core_rd.c        |    6 ------
 drivers/target/target_core_transport.c |   12 ++++++------
 include/target/target_core_transport.h |   12 ++++++++++--
 5 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index dd7abef..adac070 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -774,6 +774,20 @@ static int fd_do_task(struct se_task *task)
 	return PYX_TRANSPORT_SENT_TO_TRANSPORT;
 }
 
+static int fd_do_discard(struct se_task *task, enum blk_discard_type type)
+{
+	if (type == DISCARD_UNMAP)
+		return fd_emulate_unmap(task);
+	else if (type == DISCARD_WRITE_SAME_UNMAP)
+		return fd_emulate_write_same_unmap(task);
+	else {
+		printk(KERN_ERR "Unsupported discard_type_t: %d\n", type);
+		return -ENOSYS;
+	}
+
+	return -ENOSYS;
+}
+
 /*	fd_free_task(): (Part of se_subsystem_api_t template)
  *
  *
@@ -1158,6 +1172,7 @@ static struct se_subsystem_api fileio_template = {
 	.transport_complete	= fd_transport_complete,
 	.allocate_request	= fd_allocate_request,
 	.do_task		= fd_do_task,
+	.do_discard		= fd_do_discard,
 	.free_task		= fd_free_task,
 	.check_configfs_dev_params = fd_check_configfs_dev_params,
 	.set_configfs_dev_params = fd_set_configfs_dev_params,
@@ -1183,8 +1198,6 @@ static struct se_subsystem_api fileio_template = {
 };
 
 static struct se_subsystem_api_cdb fileio_cdb_template = {
-	.emulate_unmap		= fd_emulate_unmap,
-	.emulate_write_same	= fd_emulate_write_same_unmap,
 	.emulate_sync_cache	= fd_emulate_sync_cache,
 };
 
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index cb53fbf..e1d945f 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -419,15 +419,6 @@ static unsigned long long iblock_emulate_read_cap_with_block_size(
 	return blocks_long;
 }
 
-static int iblock_emulate_unmap(struct se_task *task)
-{
-	struct iblock_dev *ibd = task->se_dev->dev_ptr;
-	struct block_device *bd = ibd->ibd_bd;
-	struct se_cmd *cmd = TASK_CMD(task);
-	
-	return transport_generic_unmap(cmd, bd);
-}
-
 static int iblock_emulate_write_same_unmap(struct se_task *task)
 {
 	struct iblock_dev *ibd = task->se_dev->dev_ptr;
@@ -568,6 +559,24 @@ static int iblock_do_task(struct se_task *task)
 	return PYX_TRANSPORT_SENT_TO_TRANSPORT;
 }
 
+static int iblock_do_discard(struct se_task *task, enum blk_discard_type type)
+{
+	struct iblock_dev *ibd = task->se_dev->dev_ptr;
+	struct block_device *bd = ibd->ibd_bd;
+	struct se_cmd *cmd = TASK_CMD(task);
+
+	if (type == DISCARD_UNMAP)
+		return transport_generic_unmap(cmd, bd);	
+	else if (type == DISCARD_WRITE_SAME_UNMAP)
+		return iblock_emulate_write_same_unmap(task);	
+	else {
+		printk(KERN_ERR "Unsupported discard_type_t: %d\n", type);
+		return -ENOSYS;
+	}
+
+	return -ENOSYS;
+}
+
 static void iblock_free_task(struct se_task *task)
 {
 	struct iblock_req *req = task->transport_req;
@@ -1078,6 +1087,7 @@ static struct se_subsystem_api iblock_template = {
 	.transport_complete	= iblock_transport_complete,
 	.allocate_request	= iblock_allocate_request,
 	.do_task		= iblock_do_task,
+	.do_discard		= iblock_do_discard,
 	.free_task		= iblock_free_task,
 	.check_configfs_dev_params = iblock_check_configfs_dev_params,
 	.set_configfs_dev_params = iblock_set_configfs_dev_params,
@@ -1104,8 +1114,6 @@ static struct se_subsystem_api iblock_template = {
 };
 
 static struct se_subsystem_api_cdb iblock_cdb_template = {
-	.emulate_unmap		= iblock_emulate_unmap,
-	.emulate_write_same	= iblock_emulate_write_same_unmap,
 	.emulate_sync_cache	= iblock_emulate_sync_cache,
 };
 
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index c84213f..61f4a1c 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -1433,18 +1433,12 @@ static struct se_subsystem_api rd_mcp_template = {
 	.write_pending		= NULL,
 };
 
-static struct se_subsystem_api_cdb rd_cdb_template = {
-	.emulate_unmap		= NULL,
-};
-
 int __init rd_module_init(void)
 {
 	int ret;
 
 	INIT_LIST_HEAD(&rd_dr_template.sub_api_list);
-	rd_dr_template.sub_cdb = &rd_cdb_template;
 	INIT_LIST_HEAD(&rd_mcp_template.sub_api_list);
-	rd_mcp_template.sub_cdb = &rd_cdb_template;
 
 	ret = transport_subsystem_register(&rd_dr_template, NULL);
 	if (ret < 0)
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 3fb56d5..469f46f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -5606,22 +5606,22 @@ int transport_emulate_control_cdb(struct se_task *task)
 			return ret;
 		break;
 	case UNMAP:
-		if (!(api_cdb->emulate_unmap)) {
+		if (!(TRANSPORT(dev)->do_discard)) {
 			printk(KERN_ERR "UNMAP emulation not supported for: %s\n",
 					TRANSPORT(dev)->name);
 			return PYX_TRANSPORT_UNKNOWN_SAM_OPCODE;
 		}
-		ret = api_cdb->emulate_unmap(task);
+		ret = TRANSPORT(dev)->do_discard(task, DISCARD_UNMAP);
 		if (ret < 0)
 			return ret;
 		break;
 	case WRITE_SAME_16:
-		if (!(api_cdb->emulate_write_same)) {
+		if (!(TRANSPORT(dev)->do_discard)) {
 			printk(KERN_ERR "WRITE_SAME_16 emulation not supported"
 					" for: %s\n", TRANSPORT(dev)->name);
 			return PYX_TRANSPORT_UNKNOWN_SAM_OPCODE;
 		}
-		ret = api_cdb->emulate_write_same(task);
+		ret = TRANSPORT(dev)->do_discard(task, DISCARD_WRITE_SAME_UNMAP);
 		if (ret < 0)
 			return ret;
 		break;
@@ -5629,12 +5629,12 @@ int transport_emulate_control_cdb(struct se_task *task)
 		service_action = get_unaligned_be16(&T_TASK(cmd)->t_task_cdb[8]);
 		switch (service_action) {
 		case WRITE_SAME_32:
-			if (!(api_cdb->emulate_write_same)) {
+			if (!(TRANSPORT(dev)->do_discard)) {
 				printk(KERN_ERR "WRITE_SAME_32 SA emulation not"
 					" supported for: %s\n", TRANSPORT(dev)->name);
 				return PYX_TRANSPORT_UNKNOWN_SAM_OPCODE;
 			}
-			ret = api_cdb->emulate_write_same(task);
+			ret = TRANSPORT(dev)->do_discard(task, DISCARD_WRITE_SAME_UNMAP);
 			if (ret < 0)
 				return ret;
 			break;
diff --git a/include/target/target_core_transport.h b/include/target/target_core_transport.h
index 1c62d8c..dd0448b 100644
--- a/include/target/target_core_transport.h
+++ b/include/target/target_core_transport.h
@@ -127,6 +127,11 @@
 
 #define MOD_MAX_SECTORS(ms, bs)			(ms % (PAGE_SIZE / bs))
 
+enum blk_discard_type {
+	DISCARD_UNMAP,
+	DISCARD_WRITE_SAME_UNMAP,
+};
+
 struct se_mem;
 struct se_subsystem_api;
 
@@ -318,8 +323,6 @@ struct se_mem {
  * subsystem plugins.for those CDBs that cannot be emulated generically.
  */
 struct se_subsystem_api_cdb {
-	int (*emulate_unmap)(struct se_task *);
-	int (*emulate_write_same)(struct se_task *);
 	void (*emulate_sync_cache)(struct se_task *);
 };
 
@@ -478,6 +481,11 @@ struct se_subsystem_api {
 	 */
 	int (*do_task)(struct se_task *);
 	/*
+	 * Used by virtual subsystem plugins IBLOCK and FILEIO to emulate
+	 * UNMAP and WRITE_SAME_* w/ UNMAP=1 <-> Linux/Block Discard
+	 */
+	int (*do_discard)(struct se_task *, enum blk_discard_type);
+	/*
 	 * free_task():
 	 */
 	void (*free_task)(struct se_task *);
-- 
1.5.6.5

Re: [PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling

[Christoph Hellwig]

> +static int iblock_do_discard(struct se_task *task, enum blk_discard_type type)
> +{
> +	struct iblock_dev *ibd = task->se_dev->dev_ptr;
> +	struct block_device *bd = ibd->ibd_bd;
> +	struct se_cmd *cmd = TASK_CMD(task);
> +
> +	if (type == DISCARD_UNMAP)
> +		return transport_generic_unmap(cmd, bd);	
> +	else if (type == DISCARD_WRITE_SAME_UNMAP)
> +		return iblock_emulate_write_same_unmap(task);	
> +	else {
> +		printk(KERN_ERR "Unsupported discard_type_t: %d\n", type);
> +		return -ENOSYS;
> +	}
> +
> +	return -ENOSYS;
> +}
> +

I don't think the discard code is quite, nor nicely structured.

The parsing of the WRITE SAME and UNMAP CDBs is something the generic
CDB parsing code should do, and just give a range of lists of lba/len
pairs to the ->discard method in the backed.  Also your generic
discard helpers aren't actually generic - they require a block device
and thus should be only in iblock.c.  While your hack in the file
backend to use it if we're using a block device as backing file
works it's rather gross.  Having the file backend general enough to
work with a block devices is fine, but adding special hacks that
only work on block device while having a fully working bio based backed
is a bit gross.  Btw, at least on XFS you can implement discard using
hole punch operations, although that can lead to quite bad fragmentation
in cases.  Just as block-level discards can lead to quite bad
performance - I'd suggest to not enable them by default.

One other thing I noticed is that you use igrab a lot.  In general
drivers have absolutely no need for a igrab.  If you have a reference
to the file behind an inode you keep the inode in core and there's no
need at all to grab a second reference to it.

Re: [PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling

[Nicholas A. Bellinger]

On Wed, 2010-10-13 at 13:19 +0200, Christoph Hellwig wrote:
> > +static int iblock_do_discard(struct se_task *task, enum blk_discard_type type)
> > +{
> > +	struct iblock_dev *ibd = task->se_dev->dev_ptr;
> > +	struct block_device *bd = ibd->ibd_bd;
> > +	struct se_cmd *cmd = TASK_CMD(task);
> > +
> > +	if (type == DISCARD_UNMAP)
> > +		return transport_generic_unmap(cmd, bd);	
> > +	else if (type == DISCARD_WRITE_SAME_UNMAP)
> > +		return iblock_emulate_write_same_unmap(task);	
> > +	else {
> > +		printk(KERN_ERR "Unsupported discard_type_t: %d\n", type);
> > +		return -ENOSYS;
> > +	}
> > +
> > +	return -ENOSYS;
> > +}
> > +
> 
> I don't think the discard code is quite, nor nicely structured.
> 
> The parsing of the WRITE SAME and UNMAP CDBs is something the generic
> CDB parsing code should do,

Ok, so you are thinking about a seperate transport_emulate_write_same()
and transport_emulate_unmap() called from
transport_emulate_control_cdb(), right..?

>  and just give a range of lists of lba/len
> pairs to the ->discard method in the backed.

Yes, these are already available from the passed struct
se_task->task_lba and ->task_size values.

>  Also your generic
> discard helpers aren't actually generic - they require a block device
> and thus should be only in iblock.c.  While your hack in the file
> backend to use it if we're using a block device as backing file
> works it's rather gross.  Having the file backend general enough to
> work with a block devices is fine, but adding special hacks that
> only work on block device while having a fully working bio based backed
> is a bit gross.

Yes, so the problem of trying to make this code generic (eg: outside of
TCM subsystem plugins) is that blk_issue_discard() takes struct
block_device, which means we the subsystem plugin has to locate struct
block_device inside of non generic cide.

So, then the main issue becomes FILEIO + block level discard and how to
issue an blk_issue_discard() from struct fileio in the most sane way.
If there is no sane way then I will just drop this bit, or just do the
file level 'hole punch' that you are speaking about.

> Btw, at least on XFS you can implement discard using
> hole punch operations, although that can lead to quite bad fragmentation
> in cases.  Just as block-level discards can lead to quite bad
> performance - I'd suggest to not enable them by default.
> 

Ok, I will disable these by default for IBLOCK and FILEIO, and require
an explict override from user with the emulate_tpu and emulate_tpws
values in /sys/kernel/config/target/core/$HBA/$DEV/attrib/

> One other thing I noticed is that you use igrab a lot.  In general
> drivers have absolutely no need for a igrab.  If you have a reference
> to the file behind an inode you keep the inode in core and there's no
> need at all to grab a second reference to it.

The igrab() and iput() usage in FILEIO code are used for locating the
struct block_device for blk_issue_discard().  If this is the case then I
will remove these from FILEIO.

Thanks!

--nab

Re: [PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling

[Christoph Hellwig]

On Wed, Oct 13, 2010 at 01:56:08PM -0700, Nicholas A. Bellinger wrote:
> > The parsing of the WRITE SAME and UNMAP CDBs is something the generic
> > CDB parsing code should do,
> 
> Ok, so you are thinking about a seperate transport_emulate_write_same()
> and transport_emulate_unmap() called from
> transport_emulate_control_cdb(), right..?

More or less yes.

> >  and just give a range of lists of lba/len
> > pairs to the ->discard method in the backed.
> 
> Yes, these are already available from the passed struct
> se_task->task_lba and ->task_size values.

Not for UNMAP.  WRITE SAME in it's various incarnations uses the
standard LBA/LEN encoding and you seem to parse it nicely.  But for
UNMAP the lba/len pairs are in the command payload.  To support things
genericly you'd need a standard way to pass them.  If you want to
limit yourself to one lba/len pair for one the scheme could work,
though.

> Yes, so the problem of trying to make this code generic (eg: outside of
> TCM subsystem plugins) is that blk_issue_discard() takes struct
> block_device, which means we the subsystem plugin has to locate struct
> block_device inside of non generic cide.

blk_issue_discard is in no way generic.  It's 100% iblock code and
really doesn't belong into any other backend.  And btw,
blk_issue_discard is rather suboptimal even in iblock - it's a
synchronous function that will stall progress of the thread handling it.
If you want better performance you'll need to opencode the content of
it to allow an asynchronous completion handler.  But given that discard
isn't really a critical feature at this point this could easily be
left for later with a comment.

> So, then the main issue becomes FILEIO + block level discard and how to
> issue an blk_issue_discard() from struct fileio in the most sane way.
> If there is no sane way then I will just drop this bit, or just do the
> file level 'hole punch' that you are speaking about.

Right now there is no good way to do a block device discard or file
hole punch at the level where the file backend operates.

Re: [PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling

[Nicholas A. Bellinger]

On Thu, 2010-10-14 at 02:03 +0200, Christoph Hellwig wrote:
> On Wed, Oct 13, 2010 at 01:56:08PM -0700, Nicholas A. Bellinger wrote:
> > > The parsing of the WRITE SAME and UNMAP CDBs is something the generic
> > > CDB parsing code should do,
> > 
> > Ok, so you are thinking about a seperate transport_emulate_write_same()
> > and transport_emulate_unmap() called from
> > transport_emulate_control_cdb(), right..?
> 
> More or less yes.
> 

Ok, then I shall convert transport_generic_[unmap,write_same]() which
currently call blk_issue_discard() directly from IBLOCK code, and turn
the ->do_discard() subsystem API op into the LBA+Range subsystem call
with the underlying IBLOCK specific call to blk_issue_discard().

> > >  and just give a range of lists of lba/len
> > > pairs to the ->discard method in the backed.
> > 
> > Yes, these are already available from the passed struct
> > se_task->task_lba and ->task_size values.
> 
> Not for UNMAP.  WRITE SAME in it's various incarnations uses the
> standard LBA/LEN encoding and you seem to parse it nicely.  But for
> UNMAP the lba/len pairs are in the command payload.  To support things
> genericly you'd need a standard way to pass them.  If you want to
> limit yourself to one lba/len pair for one the scheme could work,
> though.
> 

Yes, this is what transport_generic_unmap() is currently doing when
called from iblock_do_discard() an walks the received UNMAP payload.

> > Yes, so the problem of trying to make this code generic (eg: outside of
> > TCM subsystem plugins) is that blk_issue_discard() takes struct
> > block_device, which means we the subsystem plugin has to locate struct
> > block_device inside of non generic cide.
> 
> blk_issue_discard is in no way generic.  It's 100% iblock code and
> really doesn't belong into any other backend.

Agreed.

> And btw,
> blk_issue_discard is rather suboptimal even in iblock - it's a
> synchronous function that will stall progress of the thread handling it.
> If you want better performance you'll need to opencode the content of
> it to allow an asynchronous completion handler.  But given that discard
> isn't really a critical feature at this point this could easily be
> left for later with a comment.
> 

I have not gotten around to the async discard caller just yet, but this
is straight-forward enough for the next round..

> > So, then the main issue becomes FILEIO + block level discard and how to
> > issue an blk_issue_discard() from struct fileio in the most sane way.
> > If there is no sane way then I will just drop this bit, or just do the
> > file level 'hole punch' that you are speaking about.
> 
> Right now there is no good way to do a block device discard or file
> hole punch at the level where the file backend operates.
> 

Understood.  In that case I will go ahead and drop the FILEIO discard
support all together for .37 code, and we can revist as necessary down
the road.

Best,

--nab

Re: [PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling

[Boaz Harrosh]

On 10/13/2010 08:03 PM, Christoph Hellwig wrote:
> On Wed, Oct 13, 2010 at 01:56:08PM -0700, Nicholas A. Bellinger wrote:
>>> The parsing of the WRITE SAME and UNMAP CDBs is something the generic
>>> CDB parsing code should do,
>>
>> Ok, so you are thinking about a seperate transport_emulate_write_same()
>> and transport_emulate_unmap() called from
>> transport_emulate_control_cdb(), right..?
> 
> More or less yes.
> 
>>>  and just give a range of lists of lba/len
>>> pairs to the ->discard method in the backed.
>>
>> Yes, these are already available from the passed struct
>> se_task->task_lba and ->task_size values.
> 
> Not for UNMAP.  WRITE SAME in it's various incarnations uses the
> standard LBA/LEN encoding and you seem to parse it nicely.  But for
> UNMAP the lba/len pairs are in the command payload.  To support things
> genericly you'd need a standard way to pass them.  If you want to
> limit yourself to one lba/len pair for one the scheme could work,
> though.
> 
>> Yes, so the problem of trying to make this code generic (eg: outside of
>> TCM subsystem plugins) is that blk_issue_discard() takes struct
>> block_device, which means we the subsystem plugin has to locate struct
>> block_device inside of non generic cide.
> 
> blk_issue_discard is in no way generic.  It's 100% iblock code and
> really doesn't belong into any other backend.  And btw,
> blk_issue_discard is rather suboptimal even in iblock - it's a
> synchronous function that will stall progress of the thread handling it.
> If you want better performance you'll need to opencode the content of
> it to allow an asynchronous completion handler.  But given that discard
> isn't really a critical feature at this point this could easily be
> left for later with a comment.
> 
>> So, then the main issue becomes FILEIO + block level discard and how to
>> issue an blk_issue_discard() from struct fileio in the most sane way.
>> If there is no sane way then I will just drop this bit, or just do the
>> file level 'hole punch' that you are speaking about.
> 
> Right now there is no good way to do a block device discard or file
> hole punch at the level where the file backend operates.
> 

Nick why don't you let User-mode (With configfs everything is user mode
right?) look into the file set for FILEIO and if found to be a block
device then set iblock instead. Then you can surly assume in FILEIO that
you only have FS files at hand.

Christoph.
has xfs an IOCTL to punch holes in files? would it be desirable to make
that generic with a ->punch() vectror FS(s) can implement. It's the 3rd
project that's looking for a generic punch().

Thanks
Boaz 

Re: [PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling

[Nicholas A. Bellinger]

On Sun, 2010-10-17 at 17:52 +0200, Boaz Harrosh wrote:
> On 10/13/2010 08:03 PM, Christoph Hellwig wrote:
> > On Wed, Oct 13, 2010 at 01:56:08PM -0700, Nicholas A. Bellinger wrote:
> >>> The parsing of the WRITE SAME and UNMAP CDBs is something the generic
> >>> CDB parsing code should do,
> >>
> >> Ok, so you are thinking about a seperate transport_emulate_write_same()
> >> and transport_emulate_unmap() called from
> >> transport_emulate_control_cdb(), right..?
> > 
> > More or less yes.
> > 
> >>>  and just give a range of lists of lba/len
> >>> pairs to the ->discard method in the backed.
> >>
> >> Yes, these are already available from the passed struct
> >> se_task->task_lba and ->task_size values.
> > 
> > Not for UNMAP.  WRITE SAME in it's various incarnations uses the
> > standard LBA/LEN encoding and you seem to parse it nicely.  But for
> > UNMAP the lba/len pairs are in the command payload.  To support things
> > genericly you'd need a standard way to pass them.  If you want to
> > limit yourself to one lba/len pair for one the scheme could work,
> > though.
> > 
> >> Yes, so the problem of trying to make this code generic (eg: outside of
> >> TCM subsystem plugins) is that blk_issue_discard() takes struct
> >> block_device, which means we the subsystem plugin has to locate struct
> >> block_device inside of non generic cide.
> > 
> > blk_issue_discard is in no way generic.  It's 100% iblock code and
> > really doesn't belong into any other backend.  And btw,
> > blk_issue_discard is rather suboptimal even in iblock - it's a
> > synchronous function that will stall progress of the thread handling it.
> > If you want better performance you'll need to opencode the content of
> > it to allow an asynchronous completion handler.  But given that discard
> > isn't really a critical feature at this point this could easily be
> > left for later with a comment.
> > 
> >> So, then the main issue becomes FILEIO + block level discard and how to
> >> issue an blk_issue_discard() from struct fileio in the most sane way.
> >> If there is no sane way then I will just drop this bit, or just do the
> >> file level 'hole punch' that you are speaking about.
> > 
> > Right now there is no good way to do a block device discard or file
> > hole punch at the level where the file backend operates.
> > 
> 
> Nick why don't you let User-mode (With configfs everything is user mode
> right?)

Correct, everything is driven by user-mode code -> syscalls and
translated into kernel level operations using Linux/VFS to represent
struct config_group dependencies between parent/child directory layout
and inter module symlinks.

> look into the file set for FILEIO and if found to be a block
> device then set iblock instead. Then you can surly assume in FILEIO that
> you only have FS files at hand.
> 

Yes, I don't think we ever want to do that type of auto-configuration at
the kernel level because using interpreted userspace code to drive
configfs is oh so much easier to develop and maintain for the long run.

> Christoph.
> has xfs an IOCTL to punch holes in files? would it be desirable to make
> that generic with a ->punch() vectror FS(s) can implement. It's the 3rd
> project that's looking for a generic punch().
> 

This sounds really interesting, any thoughts hch..?

--nab