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

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

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