| Message ID | 1473369130-22986-4-git-send-email-jbacik@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-09-08 at 17:12 -0400, Josef Bacik wrote: > In preparation for some future changes, change a few of the state bools over to > normal bits to set/clear properly. [] > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c [] > @@ -41,8 +41,12 @@ > > #include <linux/nbd.h> > > +#define NBD_TIMEDOUT 0 > +#define NBD_DISCONNECT_REQUESTED 1 > + > struct nbd_device { > u32 flags; > + unsigned long runtime_flags; Better to use DECLARE_BITMAP > @@ -626,7 +627,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, > return -EINVAL; > } > > - nbd->disconnect = true; > + set_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags); And remove the & from runtime_flags here -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/08/2016 07:20 PM, Joe Perches wrote: > On Thu, 2016-09-08 at 17:12 -0400, Josef Bacik wrote: >> In preparation for some future changes, change a few of the state bools over to >> normal bits to set/clear properly. > [] >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > [] >> @@ -41,8 +41,12 @@ >> >> #include <linux/nbd.h> >> >> +#define NBD_TIMEDOUT 0 >> +#define NBD_DISCONNECT_REQUESTED 1 >> + >> struct nbd_device { >> u32 flags; >> + unsigned long runtime_flags; > > Better to use DECLARE_BITMAP It's a few flags, we know it fits in a long. There's no point to using anything but that, and set/test/clear_bit().
On Fri, 2016-09-09 at 07:55 -0600, Jens Axboe wrote: > On 09/08/2016 07:20 PM, Joe Perches wrote: > > On Thu, 2016-09-08 at 17:12 -0400, Josef Bacik wrote: > > > In preparation for some future changes, change a few of the state bools over to > > > normal bits to set/clear properly. > > [] > > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > > [] > > > @@ -41,8 +41,12 @@ > > > > > > #include <linux/nbd.h> > > > > > > +#define NBD_TIMEDOUT 0 > > > +#define NBD_DISCONNECT_REQUESTED 1 > > > + > > > struct nbd_device { > > > u32 flags; > > + unsigned long runtime_flags; > > Better to use DECLARE_BITMAP > It's a few flags, we know it fits in a long. There's no point to using > anything but that, and set/test/clear_bit(). It lets the reader know how it's used. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2016 10:04 AM, Joe Perches wrote: > On Fri, 2016-09-09 at 07:55 -0600, Jens Axboe wrote: >> On 09/08/2016 07:20 PM, Joe Perches wrote: >>> On Thu, 2016-09-08 at 17:12 -0400, Josef Bacik wrote: >>>> In preparation for some future changes, change a few of the state bools over to >>>> normal bits to set/clear properly. >>> [] >>>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c >>> [] >>>> @@ -41,8 +41,12 @@ >>>> >>>> #include <linux/nbd.h> >>>> >>>> +#define NBD_TIMEDOUT 0 >>>> +#define NBD_DISCONNECT_REQUESTED 1 >>>> + >>>> struct nbd_device { >>>> u32 flags; >>> + unsigned long runtime_flags; >>> Better to use DECLARE_BITMAP > >> It's a few flags, we know it fits in a long. There's no point to using >> anything but that, and set/test/clear_bit(). > > It lets the reader know how it's used. The variable is called 'runtime_flags' - if that doesn't already tell the reader how it's used, then I'd suggest the reader go read something else. I'm all for using established APIs where it makes sense. Declaring a bitmap for a few fields isn't that.
On Fri, 2016-09-09 at 10:11 -0600, Jens Axboe wrote: > The variable is called 'runtime_flags' - if that doesn't already tell > the reader how it's used, then I'd suggest the reader go read something > else. > > I'm all for using established APIs where it makes sense. Declaring a > bitmap for a few fields isn't that. Deviating from established APIs makes no sense. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2016 10:15 AM, Joe Perches wrote: > On Fri, 2016-09-09 at 10:11 -0600, Jens Axboe wrote: >> The variable is called 'runtime_flags' - if that doesn't already tell >> the reader how it's used, then I'd suggest the reader go read something >> else. >> >> I'm all for using established APIs where it makes sense. Declaring a >> bitmap for a few fields isn't that. > > Deviating from established APIs makes no sense. Look, doing set/test/clear bit on an unsigned long is a very well established API. We've been doing that _forever_. As I said in my original email, I have nothing against using bitmaps _where they make sense_. Manually allocating an array for X number of bits (where X > 32), yes, by all means use the bitmap helpers. This is my final reply on this topic.
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 4b7d0f3..cf855a1 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -41,8 +41,12 @@ #include <linux/nbd.h> +#define NBD_TIMEDOUT 0 +#define NBD_DISCONNECT_REQUESTED 1 + struct nbd_device { u32 flags; + unsigned long runtime_flags; struct socket * sock; /* If == NULL, device is not ready, yet */ int magic; @@ -54,8 +58,6 @@ struct nbd_device { int blksize; loff_t bytesize; int xmit_timeout; - bool timedout; - bool disconnect; /* a disconnect has been requested by user */ struct timer_list timeout_timer; /* protects initialization and shutdown of the socket */ @@ -192,7 +194,7 @@ static void nbd_xmit_timeout(unsigned long arg) spin_lock_irqsave(&nbd->sock_lock, flags); - nbd->timedout = true; + set_bit(NBD_TIMEDOUT, &nbd->runtime_flags); if (nbd->sock) { sock = nbd->sock; @@ -562,8 +564,7 @@ out: /* Reset all properties of an NBD device */ static void nbd_reset(struct nbd_device *nbd) { - nbd->disconnect = false; - nbd->timedout = false; + nbd->runtime_flags = 0; nbd->blksize = 1024; nbd->bytesize = 0; set_capacity(nbd->disk, 0); @@ -626,7 +627,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, return -EINVAL; } - nbd->disconnect = true; + set_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags); nbd_send_cmd(nbd, blk_mq_rq_to_pdu(sreq)); blk_mq_free_request(sreq); @@ -706,9 +707,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, kill_bdev(bdev); nbd_bdev_reset(bdev); - if (nbd->disconnect) /* user requested, ignore socket errors */ + /* user requested, ignore socket errors */ + if (test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags)) error = 0; - if (nbd->timedout) + if (test_bit(NBD_TIMEDOUT, &nbd->runtime_flags)) error = -ETIMEDOUT; nbd_reset(nbd);
In preparation for some future changes, change a few of the state bools over to normal bits to set/clear properly. Signed-off-by: Josef Bacik <jbacik@fb.com> --- drivers/block/nbd.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)