lkml.org 
[lkml]   [2010]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1 subsystem plugin handling
From
Date
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






\
 
 \ /
  Last update: 2010-10-14 06:35    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site