Re: [PATCH v4] sg: O_EXCL and other lock handling

[Douglas Gilbert <dgilbert@interlog.com>]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

On 14-06-13 05:03 AM, Christoph Hellwig wrote:
> Hi Doug,
>
> this looks generally good to me, but I don't think open_cnt and exclude
> have to use atomic_t, as they are only ever modified under open_rel_lock.
>
> Can you take a look at the version below?  This changes open_cnt to an
> int, exclude to a bool, removes the open_cnt underflow check that
> the VFS takes care for, and streamlines the open path a little bit:

As a follow-up attached is a first cut of replacing the locks
around sg_request::done with an atomic. It assumes the patch
from Christoph earlier in this thread. The interesting part is
the removal of the write lock in the completion part of the
SG_IO ioctl.

Testing ongoing.

Doug Gilbert


[-- Attachment #2: sg_done_atomic1.patch --]
[-- Type: text/x-patch, Size: 6249 bytes --]

--- a/drivers/scsi/sg.c	2014-06-13 09:39:54.962003585 -0400
+++ b/drivers/scsi/sg.c	2014-06-13 13:11:49.796632281 -0400
@@ -138,7 +138,7 @@ typedef struct sg_request {	/* SG_MAX_QU
 	char orphan;		/* 1 -> drop on sight, 0 -> normal */
 	char sg_io_owned;	/* 1 -> packet belongs to SG_IO */
 	/* done protected by rq_list_lock */
-	char done;		/* 0->before bh, 1->before read, 2->read */
+	atomic_t done;		/* 0->before bh, 1->before read, 2->read */
 	struct request *rq;
 	struct bio *bio;
 	struct execute_work ew;
@@ -809,17 +809,6 @@ sg_common_write(Sg_fd * sfp, Sg_request
 	return 0;
 }
 
-static int srp_done(Sg_fd *sfp, Sg_request *srp)
-{
-	unsigned long flags;
-	int ret;
-
-	read_lock_irqsave(&sfp->rq_list_lock, flags);
-	ret = srp->done;
-	read_unlock_irqrestore(&sfp->rq_list_lock, flags);
-	return ret;
-}
-
 static long
 sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {
@@ -851,16 +840,17 @@ sg_ioctl(struct file *filp, unsigned int
 		if (result < 0)
 			return result;
 		result = wait_event_interruptible(sfp->read_wait,
-			(srp_done(sfp, srp) || atomic_read(&sdp->detaching)));
+				(atomic_read(&srp->done) ||
+				 atomic_read(&sdp->detaching)));
 		if (atomic_read(&sdp->detaching))
 			return -ENODEV;
-		write_lock_irq(&sfp->rq_list_lock);
-		if (srp->done) {
-			srp->done = 2;
-			write_unlock_irq(&sfp->rq_list_lock);
+		if (likely(atomic_add_unless(&srp->done, 1, 0))) {
+			/* srp->done bumped from 1 to 2 which is expected */
 			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
 			return (result < 0) ? result : 0;
 		}
+		/* here if srp->done was 0, abnormal */
+		write_lock_irq(&sfp->rq_list_lock);
 		srp->orphan = 1;
 		write_unlock_irq(&sfp->rq_list_lock);
 		return result;	/* -ERESTARTSYS because signal hit process */
@@ -932,7 +922,8 @@ sg_ioctl(struct file *filp, unsigned int
 			return -EFAULT;
 		read_lock_irqsave(&sfp->rq_list_lock, iflags);
 		for (srp = sfp->headrp; srp; srp = srp->nextrp) {
-			if ((1 == srp->done) && (!srp->sg_io_owned)) {
+			if ((1 == atomic_read(&srp->done)) &&
+			    (!srp->sg_io_owned)) {
 				read_unlock_irqrestore(&sfp->rq_list_lock,
 						       iflags);
 				__put_user(srp->header.pack_id, ip);
@@ -945,7 +936,8 @@ sg_ioctl(struct file *filp, unsigned int
 	case SG_GET_NUM_WAITING:
 		read_lock_irqsave(&sfp->rq_list_lock, iflags);
 		for (val = 0, srp = sfp->headrp; srp; srp = srp->nextrp) {
-			if ((1 == srp->done) && (!srp->sg_io_owned))
+			if ((1 == atomic_read(&srp->done)) &&
+			    (!srp->sg_io_owned))
 				++val;
 		}
 		read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
@@ -1015,12 +1007,13 @@ sg_ioctl(struct file *filp, unsigned int
 			     ++val, srp = srp ? srp->nextrp : srp) {
 				memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
 				if (srp) {
-					rinfo[val].req_state = srp->done + 1;
+					rinfo[val].req_state =
+						atomic_read(&srp->done) + 1;
 					rinfo[val].problem =
-					    srp->header.masked_status & 
-					    srp->header.host_status & 
-					    srp->header.driver_status;
-					if (srp->done)
+						srp->header.masked_status & 
+						srp->header.host_status & 
+						srp->header.driver_status;
+					if (atomic_read(&srp->done))
 						rinfo[val].duration =
 							srp->header.duration;
 					else {
@@ -1173,7 +1166,8 @@ sg_poll(struct file *filp, poll_table *
 	read_lock_irqsave(&sfp->rq_list_lock, iflags);
 	for (srp = sfp->headrp; srp; srp = srp->nextrp) {
 		/* if any read waiting, flag it */
-		if ((0 == res) && (1 == srp->done) && (!srp->sg_io_owned))
+		if ((0 == res) && (1 == atomic_read(&srp->done)) &&
+		    (!srp->sg_io_owned))
 			res = POLLIN | POLLRDNORM;
 		++count;
 	}
@@ -1303,7 +1297,7 @@ sg_rq_end_io(struct request *rq, int upt
 	char *sense;
 	int result, resid, done = 1;
 
-	if (WARN_ON(srp->done != 0))
+	if (WARN_ON(atomic_read(&srp->done) != 0))
 		return;
 
 	sfp = srp->parentfp;
@@ -1318,8 +1312,8 @@ sg_rq_end_io(struct request *rq, int upt
 	result = rq->errors;
 	resid = rq->resid_len;
 
-	SCSI_LOG_TIMEOUT(4, printk("sg_cmd_done: %s, pack_id=%d, res=0x%x\n",
-		sdp->disk->disk_name, srp->header.pack_id, result));
+	SCSI_LOG_TIMEOUT(4, printk("%s: %s, pack_id=%d, res=0x%x\n",
+		__func__, sdp->disk->disk_name, srp->header.pack_id, result));
 	srp->header.resid = resid;
 	ms = jiffies_to_msecs(jiffies);
 	srp->header.duration = (ms > srp->header.duration) ?
@@ -1358,7 +1352,7 @@ sg_rq_end_io(struct request *rq, int upt
 		else
 			done = 0;
 	}
-	srp->done = done;
+	atomic_set(&srp->done, done);
 	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 
 	if (likely(done)) {
@@ -1999,9 +1993,10 @@ sg_get_rq_mark(Sg_fd * sfp, int pack_id)
 	write_lock_irqsave(&sfp->rq_list_lock, iflags);
 	for (resp = sfp->headrp; resp; resp = resp->nextrp) {
 		/* look for requests that are ready + not SG_IO owned */
-		if ((1 == resp->done) && (!resp->sg_io_owned) &&
+		if ((1 == atomic_read(&resp->done)) && (!resp->sg_io_owned) &&
 		    ((-1 == pack_id) || (resp->header.pack_id == pack_id))) {
-			resp->done = 2;	/* guard against other readers */
+			/* guard against other readers */
+			atomic_set(&resp->done, 2);
 			break;
 		}
 	}
@@ -2047,6 +2042,7 @@ sg_add_request(Sg_fd * sfp)
 	if (resp) {
 		resp->nextrp = NULL;
 		resp->header.duration = jiffies_to_msecs(jiffies);
+		atomic_set(&resp->done, 0);
 	}
 	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 	return resp;
@@ -2556,7 +2552,7 @@ static int sg_proc_seq_show_devstrs(stru
 /* must be called while holding sg_index_lock */
 static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 {
-	int k, m, new_interface, blen, usg;
+	int k, m, new_interface, blen, usg, done;
 	Sg_request *srp;
 	Sg_fd *fp;
 	const sg_io_hdr_t *hp;
@@ -2596,12 +2592,12 @@ static void sg_proc_debug_helper(struct
 			seq_puts(s, cp);
 			blen = srp->data.bufflen;
 			usg = srp->data.k_use_sg;
-			seq_puts(s, srp->done ?
-				 ((1 == srp->done) ?  "rcv:" : "fin:")
-				  : "act:");
+			done = atomic_read(&srp->done);
+			seq_puts(s, done ?  ((1 == done) ?  "rcv:" : "fin:")
+					 : "act:");
 			seq_printf(s, " id=%d blen=%d",
 				   srp->header.pack_id, blen);
-			if (srp->done)
+			if (done)
 				seq_printf(s, " dur=%d", hp->duration);
 			else {
 				ms = jiffies_to_msecs(jiffies);