Re: [PATCH v4] sg: O_EXCL and other lock handling
On Fri, Jun 13, 2014 at 10:33:37AM -0400, Douglas Gilbert wrote: 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. They are read by 'cat /proc/scsi/sg/debug' [in sg_proc_seq_show_debug()] but transient off-by-1 reports are not so important. [Prior to locking that block with read_lock(sfd_lock) that routine sometimes printed wild results, so some care is required.] In general 32-bit variables that can be read independly only need the lock for updates. A lock on the read side is important if variables are of a type that can't be atomically read (e.g. 64-bit on 32-bit architectures, or small integers on some old alpha CPUs), or if the values of multiple variables need to be consistent for the reader. 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: sg_open() and sg_release() clean-up looks good. The back-up goto at the end of sg_open() reminds me of Fortran style :-) More importantly my sg_tst_excl* tests give this version the thumbs up. Acked-by: Douglas Gilbert dgilb...@interlog.com Given that this was your patch with minor changes I was planning to put this in under your name if that's fine with you. I'm also looking for another review. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] sg: O_EXCL and other lock handling
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:
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index e75f71e..0cd77a3 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -51,6 +51,7 @@ static int sg_version_num = 30534;/* 2 digits for each
component */
#include linux/delay.h
#include linux/blktrace_api.h
#include linux/mutex.h
+#include linux/atomic.h
#include linux/ratelimit.h
#include scsi.h
@@ -102,18 +103,16 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
#define SG_SECTOR_SZ 512
-static int sg_add(struct device *, struct class_interface *);
-static void sg_remove(struct device *, struct class_interface *);
-
-static DEFINE_SPINLOCK(sg_open_exclusive_lock);
+static int sg_add_device(struct device *, struct class_interface *);
+static void sg_remove_device(struct device *, struct class_interface *);
static DEFINE_IDR(sg_index_idr);
static DEFINE_RWLOCK(sg_index_lock); /* Also used to lock
file descriptor list
for device */
static struct class_interface sg_interface = {
- .add_dev= sg_add,
- .remove_dev = sg_remove,
+ .add_dev= sg_add_device,
+ .remove_dev = sg_remove_device,
};
typedef struct sg_scatter_hold { /* holding area for scsi scatter gather info
*/
@@ -146,8 +145,7 @@ typedef struct sg_request { /* SG_MAX_QUEUE requests
outstanding per file */
} Sg_request;
typedef struct sg_fd { /* holds the state of a file descriptor */
- /* sfd_siblings is protected by sg_index_lock */
- struct list_head sfd_siblings;
+ struct list_head sfd_siblings; /* protected by device's sfd_lock */
struct sg_device *parentdp; /* owning device */
wait_queue_head_t read_wait;/* queue read until command done */
rwlock_t rq_list_lock; /* protect access to list in req_arr */
@@ -170,14 +168,15 @@ typedef struct sg_fd {/* holds the state of a
file descriptor */
typedef struct sg_device { /* holds the state of each scsi generic device */
struct scsi_device *device;
- wait_queue_head_t o_excl_wait; /* queue open() when O_EXCL in use */
+ wait_queue_head_t open_wait;/* queue open() when O_EXCL present */
+ struct mutex open_rel_lock; /* held when in open() or release() */
int sg_tablesize; /* adapter's max scatter-gather table size */
u32 index; /* device index number */
- /* sfds is protected by sg_index_lock */
struct list_head sfds;
- volatile char detached; /* 0-attached, 1-detached pending removal */
- /* exclude protected by sg_open_exclusive_lock */
- char exclude; /* opened for exclusive access */
+ rwlock_t sfd_lock; /* protect access to sfd list */
+ atomic_t detaching; /* 0-device usable, 1-device detaching */
+ bool exclude; /* 1-open(O_EXCL) succeeded and is active */
+ int open_cnt; /* count of opens (perhaps num(sfds) ) */
char sgdebug; /* 0-off, 1-sense, 9-dump dev, 10- all devs
*/
struct gendisk *disk;
struct cdev * cdev; /* char_dev [sysfs: /sys/cdev/major/sgn] */
@@ -208,7 +207,7 @@ static Sg_request *sg_add_request(Sg_fd * sfp);
static int sg_remove_request(Sg_fd * sfp, Sg_request * srp);
static int sg_res_in_use(Sg_fd * sfp);
static Sg_device *sg_get_dev(int dev);
-static void sg_put_dev(Sg_device *sdp);
+static void sg_device_destroy(struct kref *kref);
#define SZ_SG_HEADER sizeof(struct sg_header)
#define SZ_SG_IO_HDR sizeof(sg_io_hdr_t)
@@ -225,38 +224,43 @@ static int sg_allow_access(struct file *filp, unsigned
char *cmd)
return blk_verify_command(cmd, filp-f_mode FMODE_WRITE);
}
-static int get_exclude(Sg_device *sdp)
-{
- unsigned long flags;
- int ret;
-
- spin_lock_irqsave(sg_open_exclusive_lock, flags);
- ret = sdp-exclude;
- spin_unlock_irqrestore(sg_open_exclusive_lock, flags);
- return ret;
-}
-
-static int set_exclude(Sg_device *sdp, char val)
+static int
+open_wait(Sg_device *sdp, int flags)
{
- unsigned long flags;
-
- spin_lock_irqsave(sg_open_exclusive_lock, flags);
- sdp-exclude = val;
- spin_unlock_irqrestore(sg_open_exclusive_lock, flags);
- return val;
-}
+ int retval = 0;
-static int sfds_list_empty(Sg_device *sdp)
-{
- unsigned long flags;
- int ret;
+ if (flags O_EXCL) {
+ while (sdp-open_cnt 0) {
+ mutex_unlock(sdp-open_rel_lock);
+ retval =
Re: [PATCH v4] sg: O_EXCL and other lock handling
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. They are read by 'cat /proc/scsi/sg/debug' [in sg_proc_seq_show_debug()] but transient off-by-1 reports are not so important. [Prior to locking that block with read_lock(sfd_lock) that routine sometimes printed wild results, so some care is required.] 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: sg_open() and sg_release() clean-up looks good. The back-up goto at the end of sg_open() reminds me of Fortran style :-) More importantly my sg_tst_excl* tests give this version the thumbs up. Acked-by: Douglas Gilbert dgilb...@interlog.com -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] sg: O_EXCL and other lock handling
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
--- 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
