Skip to content

L
linux
Commit 52fa6e21 authored by Andrew Morton's avatar Andrew Morton Committed by Linus Torvalds
Browse files
Options

[PATCH] NBD: cosmetic cleanups 

From: Lou Langholtz <ldl@aros.net>

It's a helpful step in being better able to identify code inefficiencies
and problems particularly w.r.t.  locking.  It also modifies some of the
output messages for greater consistancy and better diagnostic support.

This second patch is a lead in that way to the third patch, which will
simply introduce the dprintk() debugging facility that my jumbo patch
originally had.

With the cosmetics patch and debugging enhancement (patch), it will make it
easier to fix or at least improve the locking bugs/races in NBD (that will
likely make up the fourth patch in my envisioned roadmap).
parent e0a3db1a
Hide whitespace changes
Inline Side-by-side
Showing with 104 additions and 79 deletions
drivers/block/nbd.c
View file @ 52fa6e21
...@@ -31,6 +31,7 @@ ...@@ -31,6 +31,7 @@
* 03-06-22 Make nbd work with new linux 2.5 block layer design. This fixes * 03-06-22 Make nbd work with new linux 2.5 block layer design. This fixes
* memory corruption from module removal and possible memory corruption * memory corruption from module removal and possible memory corruption
* from sending/receiving disk data. <ldl@aros.net> * from sending/receiving disk data. <ldl@aros.net>
* 03-06-23 Cosmetic changes. <ldl@aros.net>
* *
* possible FIXME: make set_sock / set_blksize / set_size / do_it one syscall * possible FIXME: make set_sock / set_blksize / set_size / do_it one syscall
* why not: would need verify_area and friends, would share yet another * why not: would need verify_area and friends, would share yet another
...@@ -101,17 +102,11 @@ static void nbd_end_request(struct request *req) ...@@ -101,17 +102,11 @@ static void nbd_end_request(struct request *req)
spin_unlock_irqrestore(q->queue_lock, flags); spin_unlock_irqrestore(q->queue_lock, flags);
} }
static int nbd_open(struct inode *inode, struct file *file)
{
struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
lo->refcnt++;
return 0;
}
/* /*
* Send or receive packet. * Send or receive packet.
*/ */
static int nbd_xmit(int send, struct socket *sock, char *buf, int size, int msg_flags) static int sock_xmit(struct socket *sock, int send, void *buf, int size,
int msg_flags)
{ {
mm_segment_t oldfs; mm_segment_t oldfs;
int result; int result;
...@@ -131,7 +126,6 @@ static int nbd_xmit(int send, struct socket *sock, char *buf, int size, int msg_ ...@@ -131,7 +126,6 @@ static int nbd_xmit(int send, struct socket *sock, char *buf, int size, int msg_
recalc_sigpending(); recalc_sigpending();
spin_unlock_irqrestore(&current->sighand->siglock, flags); spin_unlock_irqrestore(&current->sighand->siglock, flags);
do { do {
sock->sk->sk_allocation = GFP_NOIO; sock->sk->sk_allocation = GFP_NOIO;
iov.iov_base = buf; iov.iov_base = buf;
...@@ -153,7 +147,7 @@ static int nbd_xmit(int send, struct socket *sock, char *buf, int size, int msg_ ...@@ -153,7 +147,7 @@ static int nbd_xmit(int send, struct socket *sock, char *buf, int size, int msg_
if (signal_pending(current)) { if (signal_pending(current)) {
siginfo_t info; siginfo_t info;
spin_lock_irqsave(&current->sighand->siglock, flags); spin_lock_irqsave(&current->sighand->siglock, flags);
printk(KERN_WARNING "NBD (pid %d: %s) got signal %d\n", printk(KERN_WARNING "nbd (pid %d: %s) got signal %d\n",
current->pid, current->comm, current->pid, current->comm,
dequeue_signal(current, &current->blocked, &info)); dequeue_signal(current, &current->blocked, &info));
spin_unlock_irqrestore(&current->sighand->siglock, flags); spin_unlock_irqrestore(&current->sighand->siglock, flags);
...@@ -163,8 +157,8 @@ static int nbd_xmit(int send, struct socket *sock, char *buf, int size, int msg_ ...@@ -163,8 +157,8 @@ static int nbd_xmit(int send, struct socket *sock, char *buf, int size, int msg_
if (result <= 0) { if (result <= 0) {
#ifdef PARANOIA #ifdef PARANOIA
printk(KERN_ERR "NBD: %s - sock=%ld at buf=%ld, size=%d returned %d.\n", printk(KERN_ERR "nbd: %s - sock=%p at buf=%p, size=%d returned %d.\n",
send ? "send" : "receive", (long) sock, (long) buf, size, result); send? "send": "receive", sock, buf, size, result);
#endif #endif
break; break;
} }
...@@ -186,14 +180,12 @@ static inline int sock_send_bvec(struct socket *sock, struct bio_vec *bvec, ...@@ -186,14 +180,12 @@ static inline int sock_send_bvec(struct socket *sock, struct bio_vec *bvec,
{ {
int result; int result;
void *kaddr = kmap(bvec->bv_page); void *kaddr = kmap(bvec->bv_page);
result = nbd_xmit(1, sock, kaddr + bvec->bv_offset, bvec->bv_len, result = sock_xmit(sock, 1, kaddr + bvec->bv_offset, bvec->bv_len,
flags); flags);
kunmap(bvec->bv_page); kunmap(bvec->bv_page);
return result; return result;
} }
#define FAIL( s ) { printk( KERN_ERR "NBD: " s "(result %d)\n", result ); goto error_out; }
void nbd_send_req(struct nbd_device *lo, struct request *req) void nbd_send_req(struct nbd_device *lo, struct request *req)
{ {
int result, i, flags; int result, i, flags;
...@@ -201,24 +193,29 @@ void nbd_send_req(struct nbd_device *lo, struct request *req) ...@@ -201,24 +193,29 @@ void nbd_send_req(struct nbd_device *lo, struct request *req)
unsigned long size = req->nr_sectors << 9; unsigned long size = req->nr_sectors << 9;
struct socket *sock = lo->sock; struct socket *sock = lo->sock;
DEBUG("NBD: sending control, "); DEBUG("nbd: sending control, ");
request.magic = htonl(NBD_REQUEST_MAGIC); request.magic = htonl(NBD_REQUEST_MAGIC);
request.type = htonl(nbd_cmd(req)); request.type = htonl(nbd_cmd(req));
request.from = cpu_to_be64( (u64) req->sector << 9); request.from = cpu_to_be64((u64) req->sector << 9);
request.len = htonl(size); request.len = htonl(size);
memcpy(request.handle, &req, sizeof(req)); memcpy(request.handle, &req, sizeof(req));
down(&lo->tx_lock); down(&lo->tx_lock);
if (!sock || !lo->sock) { if (!sock || !lo->sock) {
printk(KERN_ERR "NBD: Attempted sendmsg to closed socket\n"); printk(KERN_ERR "%s: Attempted sendmsg to closed socket\n",
lo->disk->disk_name);
goto error_out; goto error_out;
} }
result = nbd_xmit(1, sock, (char *) &request, sizeof(request), nbd_cmd(req) == NBD_CMD_WRITE ? MSG_MORE : 0); result = sock_xmit(sock, 1, &request, sizeof(request),
if (result <= 0) (nbd_cmd(req) == NBD_CMD_WRITE)? MSG_MORE: 0);
FAIL("Sendmsg failed for control."); if (result <= 0) {
printk(KERN_ERR "%s: Sendmsg failed for control (result %d)\n",
lo->disk->disk_name, result);
goto error_out;
}
if (nbd_cmd(req) == NBD_CMD_WRITE) { if (nbd_cmd(req) == NBD_CMD_WRITE) {
struct bio *bio; struct bio *bio;
...@@ -234,8 +231,12 @@ void nbd_send_req(struct nbd_device *lo, struct request *req) ...@@ -234,8 +231,12 @@ void nbd_send_req(struct nbd_device *lo, struct request *req)
flags = MSG_MORE; flags = MSG_MORE;
DEBUG("data, "); DEBUG("data, ");
result = sock_send_bvec(sock, bvec, flags); result = sock_send_bvec(sock, bvec, flags);
if (result <= 0) if (result <= 0) {
FAIL("Send data failed."); printk(KERN_ERR "%s: Send data failed (result %d)\n",
lo->disk->disk_name,
result);
goto error_out;
}
} }
} }
} }
...@@ -272,15 +273,14 @@ static inline int sock_recv_bvec(struct socket *sock, struct bio_vec *bvec) ...@@ -272,15 +273,14 @@ static inline int sock_recv_bvec(struct socket *sock, struct bio_vec *bvec)
{ {
int result; int result;
void *kaddr = kmap(bvec->bv_page); void *kaddr = kmap(bvec->bv_page);
result = nbd_xmit(0, sock, kaddr + bvec->bv_offset, bvec->bv_len, result = sock_xmit(sock, 0, kaddr + bvec->bv_offset, bvec->bv_len,
MSG_WAITALL); MSG_WAITALL);
kunmap(bvec->bv_page); kunmap(bvec->bv_page);
return result; return result;
} }
#define HARDFAIL( s ) { printk( KERN_ERR "NBD: " s "(result %d)\n", result ); lo->harderror = result; return NULL; } /* NULL returned = something went wrong, inform userspace */
struct request *nbd_read_stat(struct nbd_device *lo) struct request *nbd_read_stat(struct nbd_device *lo)
/* NULL returned = something went wrong, inform userspace */
{ {
int result; int result;
struct nbd_reply reply; struct nbd_reply reply;
...@@ -289,18 +289,34 @@ struct request *nbd_read_stat(struct nbd_device *lo) ...@@ -289,18 +289,34 @@ struct request *nbd_read_stat(struct nbd_device *lo)
DEBUG("reading control, "); DEBUG("reading control, ");
reply.magic = 0; reply.magic = 0;
result = nbd_xmit(0, sock, (char *) &reply, sizeof(reply), MSG_WAITALL); result = sock_xmit(sock, 0, &reply, sizeof(reply), MSG_WAITALL);
if (result <= 0) if (result <= 0) {
HARDFAIL("Recv control failed."); printk(KERN_ERR "%s: Recv control failed (result %d)\n",
lo->disk->disk_name, result);
lo->harderror = result;
return NULL;
}
req = nbd_find_request(lo, reply.handle); req = nbd_find_request(lo, reply.handle);
if (req == NULL) if (req == NULL) {
HARDFAIL("Unexpected reply"); printk(KERN_ERR "%s: Unexpected reply (result %d)\n",
lo->disk->disk_name, result);
lo->harderror = result;
return NULL;
}
DEBUG("ok, "); DEBUG("ok, ");
if (ntohl(reply.magic) != NBD_REPLY_MAGIC) if (ntohl(reply.magic) != NBD_REPLY_MAGIC) {
HARDFAIL("Not enough magic."); printk(KERN_ERR "%s: Not enough magic (result %d)\n",
if (ntohl(reply.error)) lo->disk->disk_name, result);
FAIL("Other side returned error."); lo->harderror = result;
return NULL;
}
if (ntohl(reply.error)) {
printk(KERN_ERR "%s: Other side returned error (result %d)\n",
lo->disk->disk_name, result);
req->errors++;
return req;
}
if (nbd_cmd(req) == NBD_CMD_READ) { if (nbd_cmd(req) == NBD_CMD_READ) {
int i; int i;
...@@ -310,35 +326,29 @@ struct request *nbd_read_stat(struct nbd_device *lo) ...@@ -310,35 +326,29 @@ struct request *nbd_read_stat(struct nbd_device *lo)
struct bio_vec *bvec; struct bio_vec *bvec;
bio_for_each_segment(bvec, bio, i) { bio_for_each_segment(bvec, bio, i) {
result = sock_recv_bvec(sock, bvec); result = sock_recv_bvec(sock, bvec);
if (result <= 0) if (result <= 0) {
HARDFAIL("Recv data failed."); printk(KERN_ERR "%s: Recv data failed (result %d)\n",
lo->disk->disk_name,
result);
lo->harderror = result;
return NULL;
}
} }
} }
} }
DEBUG("done.\n"); DEBUG("done.\n");
return req; return req;
/* Can we get here? Yes, if other side returns error */
error_out:
req->errors++;
return req;
} }
void nbd_do_it(struct nbd_device *lo) void nbd_do_it(struct nbd_device *lo)
{ {
struct request *req; struct request *req;
while (1) { BUG_ON(lo->magic != LO_MAGIC);
req = nbd_read_stat(lo); while ((req = nbd_read_stat(lo)) != NULL)
if (!req) {
printk(KERN_ALERT "req should never be null\n" );
goto out;
}
BUG_ON(lo->magic != LO_MAGIC);
nbd_end_request(req); nbd_end_request(req);
} printk(KERN_ALERT "%s: req should never be null\n",
out: lo->disk->disk_name);
return; return;
} }
...@@ -360,7 +370,7 @@ void nbd_clear_que(struct nbd_device *lo) ...@@ -360,7 +370,7 @@ void nbd_clear_que(struct nbd_device *lo)
req->errors++; req->errors++;
nbd_end_request(req); nbd_end_request(req);
} }
} while(req); } while (req);
} }
/* /*
...@@ -370,9 +380,6 @@ void nbd_clear_que(struct nbd_device *lo) ...@@ -370,9 +380,6 @@ void nbd_clear_que(struct nbd_device *lo)
* { printk( "Warning: Ignoring result!\n"); nbd_end_request( req ); } * { printk( "Warning: Ignoring result!\n"); nbd_end_request( req ); }
*/ */
#undef FAIL
#define FAIL( s ) { printk( KERN_ERR "%s: " s "\n", req->rq_disk->disk_name ); goto error_out; }
static void do_nbd_request(request_queue_t * q) static void do_nbd_request(request_queue_t * q)
{ {
struct request *req; struct request *req;
...@@ -380,30 +387,38 @@ static void do_nbd_request(request_queue_t * q) ...@@ -380,30 +387,38 @@ static void do_nbd_request(request_queue_t * q)
while ((req = elv_next_request(q)) != NULL) { while ((req = elv_next_request(q)) != NULL) {
struct nbd_device *lo; struct nbd_device *lo;
blkdev_dequeue_request(req);
if (!(req->flags & REQ_CMD)) if (!(req->flags & REQ_CMD))
goto error_out; goto error_out;
lo = req->rq_disk->private_data; lo = req->rq_disk->private_data;
if (!lo->file) BUG_ON(lo->magic != LO_MAGIC);
FAIL("Request when not-ready."); if (!lo->file) {
printk(KERN_ERR "%s: Request when not-ready\n",
lo->disk->disk_name);
goto error_out;
}
nbd_cmd(req) = NBD_CMD_READ; nbd_cmd(req) = NBD_CMD_READ;
if (rq_data_dir(req) == WRITE) { if (rq_data_dir(req) == WRITE) {
nbd_cmd(req) = NBD_CMD_WRITE; nbd_cmd(req) = NBD_CMD_WRITE;
if (lo->flags & NBD_READ_ONLY) if (lo->flags & NBD_READ_ONLY) {
FAIL("Write on read-only"); printk(KERN_ERR "%s: Write on read-only\n",
lo->disk->disk_name);
goto error_out;
}
} }
BUG_ON(lo->magic != LO_MAGIC);
requests_in++; requests_in++;
req->errors = 0; req->errors = 0;
blkdev_dequeue_request(req);
spin_unlock_irq(q->queue_lock); spin_unlock_irq(q->queue_lock);
spin_lock(&lo->queue_lock); spin_lock(&lo->queue_lock);
if (!lo->file) { if (!lo->file) {
spin_unlock(&lo->queue_lock); spin_unlock(&lo->queue_lock);
printk(KERN_ERR "nbd: failed between accept and semaphore, file lost\n"); printk(KERN_ERR "%s: failed between accept and semaphore, file lost\n",
lo->disk->disk_name);
req->errors++; req->errors++;
nbd_end_request(req); nbd_end_request(req);
spin_lock_irq(q->queue_lock); spin_lock_irq(q->queue_lock);
...@@ -416,7 +431,8 @@ static void do_nbd_request(request_queue_t * q) ...@@ -416,7 +431,8 @@ static void do_nbd_request(request_queue_t * q)
nbd_send_req(lo, req); nbd_send_req(lo, req);
if (req->errors) { if (req->errors) {
printk(KERN_ERR "nbd: nbd_send_req failed\n"); printk(KERN_ERR "%s: nbd_send_req failed\n",
lo->disk->disk_name);
spin_lock(&lo->queue_lock); spin_lock(&lo->queue_lock);
list_del_init(&req->queuelist); list_del_init(&req->queuelist);
spin_unlock(&lo->queue_lock); spin_unlock(&lo->queue_lock);
...@@ -430,7 +446,6 @@ static void do_nbd_request(request_queue_t * q) ...@@ -430,7 +446,6 @@ static void do_nbd_request(request_queue_t * q)
error_out: error_out:
req->errors++; req->errors++;
blkdev_dequeue_request(req);
spin_unlock(q->queue_lock); spin_unlock(q->queue_lock);
nbd_end_request(req); nbd_end_request(req);
spin_lock(q->queue_lock); spin_lock(q->queue_lock);
...@@ -438,6 +453,24 @@ static void do_nbd_request(request_queue_t * q) ...@@ -438,6 +453,24 @@ static void do_nbd_request(request_queue_t * q)
return; return;
} }
static int nbd_open(struct inode *inode, struct file *file)
{
struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
lo->refcnt++;
return 0;
}
static int nbd_release(struct inode *inode, struct file *file)
{
struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
if (lo->refcnt <= 0)
printk(KERN_ALERT "%s: %s: refcount(%d) <= 0\n",
lo->disk->disk_name, __FUNCTION__, lo->refcnt);
lo->refcnt--;
/* N.B. Doesn't lo->file need an fput?? */
return 0;
}
static int nbd_ioctl(struct inode *inode, struct file *file, static int nbd_ioctl(struct inode *inode, struct file *file,
unsigned int cmd, unsigned long arg) unsigned int cmd, unsigned long arg)
{ {
...@@ -451,7 +484,7 @@ static int nbd_ioctl(struct inode *inode, struct file *file, ...@@ -451,7 +484,7 @@ static int nbd_ioctl(struct inode *inode, struct file *file,
return -EPERM; return -EPERM;
switch (cmd) { switch (cmd) {
case NBD_DISCONNECT: case NBD_DISCONNECT:
printk(KERN_INFO "NBD_DISCONNECT\n"); printk(KERN_INFO "%s: NBD_DISCONNECT\n", lo->disk->disk_name);
sreq.flags = REQ_SPECIAL; sreq.flags = REQ_SPECIAL;
nbd_cmd(&sreq) = NBD_CMD_DISC; nbd_cmd(&sreq) = NBD_CMD_DISC;
if (!lo->sock) if (!lo->sock)
...@@ -464,7 +497,8 @@ static int nbd_ioctl(struct inode *inode, struct file *file, ...@@ -464,7 +497,8 @@ static int nbd_ioctl(struct inode *inode, struct file *file,
spin_lock(&lo->queue_lock); spin_lock(&lo->queue_lock);
if (!list_empty(&lo->queue_head)) { if (!list_empty(&lo->queue_head)) {
spin_unlock(&lo->queue_lock); spin_unlock(&lo->queue_lock);
printk(KERN_ERR "nbd: Some requests are in progress -> can not turn off.\n"); printk(KERN_ERR "%s: Some requests are in progress -> can not turn off.\n",
lo->disk->disk_name);
return -EBUSY; return -EBUSY;
} }
file = lo->file; file = lo->file;
...@@ -526,7 +560,8 @@ static int nbd_ioctl(struct inode *inode, struct file *file, ...@@ -526,7 +560,8 @@ static int nbd_ioctl(struct inode *inode, struct file *file,
* there should be a more generic interface rather than * there should be a more generic interface rather than
* calling socket ops directly here */ * calling socket ops directly here */
down(&lo->tx_lock); down(&lo->tx_lock);
printk(KERN_WARNING "nbd: shutting down socket\n"); printk(KERN_WARNING "%s: shutting down socket\n",
lo->disk->disk_name);
lo->sock->ops->shutdown(lo->sock, SEND_SHUTDOWN|RCV_SHUTDOWN); lo->sock->ops->shutdown(lo->sock, SEND_SHUTDOWN|RCV_SHUTDOWN);
lo->sock = NULL; lo->sock = NULL;
up(&lo->tx_lock); up(&lo->tx_lock);
...@@ -535,7 +570,7 @@ static int nbd_ioctl(struct inode *inode, struct file *file, ...@@ -535,7 +570,7 @@ static int nbd_ioctl(struct inode *inode, struct file *file,
lo->file = NULL; lo->file = NULL;
spin_unlock(&lo->queue_lock); spin_unlock(&lo->queue_lock);
nbd_clear_que(lo); nbd_clear_que(lo);
printk(KERN_WARNING "nbd: queue cleared\n"); printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
if (file) if (file)
fput(file); fput(file);
return lo->harderror; return lo->harderror;
...@@ -553,16 +588,6 @@ static int nbd_ioctl(struct inode *inode, struct file *file, ...@@ -553,16 +588,6 @@ static int nbd_ioctl(struct inode *inode, struct file *file,
return -EINVAL; return -EINVAL;
} }
static int nbd_release(struct inode *inode, struct file *file)
{
struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
if (lo->refcnt <= 0)
printk(KERN_ALERT "nbd_release: refcount(%d) <= 0\n", lo->refcnt);
lo->refcnt--;
/* N.B. Doesn't lo->file need an fput?? */
return 0;
}
static struct block_device_operations nbd_fops = static struct block_device_operations nbd_fops =
{ {
.owner = THIS_MODULE, .owner = THIS_MODULE,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7