[illumos-Developer] NOW with zvol-only zfs diffs --> iSCSI UNMAP

[Dan McDonald]

Two people get responses.  First Eric:

On Sat, 2011-02-19 at 09:40 -0500, Eric Schrock wrote:
> I looked into F_FREESP a little more, and unfortunately it's not as
> universal as I had hoped.  This is really an argument to fcntl(),
> which itself gets translated to VOP_SPACE() under the hood (after
> checking that the file type is VREG).  So the idea of using the same
> ioctl() for both regular files and devices is bogus.  Rather than
> trying to shoehorn it in, I would create DKIOCFREE that has the
> semantics we want (start and end of range, I would guess.

There aren't a lot of numbers left in sys/dkio.h, but I'm assuming you'd want
something in the 0-50 range?

/*
 * Disk io control commands
 * Warning: some other ioctls with the DIOC prefix exist elsewhere.
 * The Generic DKIOC numbers are from	0   -  50.
 *	The Floppy Driver uses		51  - 100.
 *	The Hard Disk (except SCSI)	101 - 106.	(these are obsolete)
 *	The CDROM Driver		151 - 200.
 *	The USCSI ioctl			201 - 250.
 */
#define	DKIOC		(0x04 << 8)

> The lack of credential checks in zvol_ioctl() seems a little dubious
> to me as well.

Until you look at the permissions on the zvol's themselves:

(0)# ls -lt /dev/zvol/dsk/rpool/swap 
lrwxrwxrwx   1 root     root           0 Feb 16 15:18 /dev/zvol/dsk/rpool/swap -> ../../../..//devices/pseudo/zfs at 0:2
(0)# ls -lt /devices/pseudo/*zfs*
brw-------   1 root     sys      182,  1 Feb 20 13:58 /devices/pseudo/zfs at 0:1
crw-------   1 root     sys      182,  1 Feb 20 13:58 /devices/pseudo/zfs at 0:1,raw
brw-------   1 root     sys      182,  2 Feb 20 13:58 /devices/pseudo/zfs at 0:2
crw-------   1 root     sys      182,  2 Feb 20 13:58 /devices/pseudo/zfs at 0:2,raw
crw-rw-rw-   1 root     sys      182,  0 Feb 16 15:18 /devices/pseudo/zfs at 0:zfs

/devices/pseudo/zfs at 0:
total 0
(0)# 

I'm guessing the zvol_ioctl() is assuming permissions are set properly from
the start.

> My guess is that these checks are enforced at the dev/ldi layer, as we also
> don't have any credential checks in zvol_write(), for example.  I'd verify
> that one needs to have the device open for writing in order to issue an
> ioctl().

The zvol ioctls are dispatched from the generic zfsdev_ioctl(), if a
minor-number is set, and soft-state indicating it's not a control-device are
set.

> Thanks for working on this, it's looking much better.

I hope so.


When Eric suggested DKIOCFREE, Garrett chimed in with:


On Sat, Feb 19, 2011 at 02:58:17PM -0800, Garrett D'Amore wrote:
> I noticed the fcntl() usage myself, and thought it was a bit strange.
> 
> I would like to make sure that if we choose an ioctl, it will work with
> ordinary SCSI unmap (initiated from ZFS for example) as well -- then it
> should be a fairly straight-forward matter to extend this in sd and
> SCSA.

I take it if we want to do DKIOCFREE, we should also implement this ioctl in
sd.c, and other places!?  Wow.  That's looking like a lot more work.  Now I
don't mind that work, but it will take time for design, and possibly have
other consequences.

Plus, if I'm going to do a quick design-on-the-fly, should I just reuse one
of the structrues in dkio.h already, perhaps:

struct extpart_info {
	diskaddr_t	p_start;
	diskaddr_t	p_length;
};

?

I can certainly go in and start hammering away, but if anyone has any
design-level concerns, this might be a good time to bring them up.

Dan