Date:	Wed, 13 Oct 2010 21:27:07 -0700
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	Christoph Hellwig <hch@....de>
Cc:	linux-scsi <linux-scsi@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Mike Christie <michaelc@...wisc.edu>,
	Hannes Reinecke <hare@...e.de>,
	James Bottomley <James.Bottomley@...e.de>,
	Boaz Harrosh <bharrosh@...asas.com>,
	Jens Axboe <axboe@...nel.dk>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	Douglas Gilbert <dgilbert@...erlog.com>,
	Richard Sharpe <realrichardsharpe@...il.com>
Subject: Re: [PATCH 4/5] tcm: Unify UNMAP and WRITE_SAME w/ UNMAP=1
	subsystem plugin handling

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




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists