Re: TRIM patches

["Samuel J. Greear" <sjg@xxxxxxxxxxxx>]

On Sat, Jul 23, 2011 at 7:43 PM, Tim Bisson <bissont@mac.com> wrote:
> Hi,
>
> Here are the trim patches. I tried to break them up into functional units so that a review will hopefully be easier.
>
> I also added support for fdisk, so you can trim a whole device with -I, or just a partition using -u.
>

Functionally this looks pretty good to me, pretty simple and
straightforward -- I just have a few notes, mostly whitespace and etc.
related, which I think probably says something (read: it looks pretty
solid to me).

Missing spaces:
sys/bus/cam/scsi/scsi_da.c: bp->b_bio1.bio_offset =bytes_start;
sys/bus/cam/scsi/scsi_da.c: if(softc->disk.d_info.d_trimflag) {
sys/bus/cam/scsi/scsi_da.c: if(!softc->trim_running &&
and more following this...

In dastart:
+                        do {
+				uint64_t lba;
+				int count;
. .. this is probably fine, but I think we generally try to avoid this
and declare all variables at the top of the function, but for infinite
loops we definitely prefer while (1) {} or for (;;) {} instead of do
{} while(1);


in scsi_da.h:
+//#define IOCATADELETE            _IOW('a', 104, off_t[2])
+

Should be removed?

-----

In hammer diff,
+				printf("Erase device option selected, but sysctl (%s) "
+				       "is not enabled\n",sysctl_name);
Space after ,

Ioctl can return EIO or EINVAL at least, check for an error return in
the hammer commands trim_volume()?

-----

swap patch,

missing space:
+static int swap_on_off(char *name, int doingall,int trim);
+		if(!trim_enabled) {


-----

ufs patch (7) has inconsistent spacing (tabs vs spaces).

-----

fdisk patch, check ioctl return?


Best,
Sam