On Tue, Jan 08, 2019 at 10:30:32AM -0500, Bryan Gurney wrote:
> On Mon, Jan 7, 2019 at 7:10 PM Benjamin Marzinski <bmarzins@xxxxxxxxxx> wrote:
> >
> > On Mon, Jan 07, 2019 at 02:31:23PM -0500, Bryan Gurney wrote:
> > > +
> > > +static int dust_add_block(struct dust_device *dd, unsigned long long block)
> > > +{
> > > + struct badblock *bblock;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&dd->dust_lock, flags);
> > > + bblock = dust_rb_search(&dd->badblocklist, block * dd->sect_per_block);
> > > +
> > > + if (bblock != NULL) {
> > > + if (!dd->quiet_mode)
> > > + DMERR("addbadblock: block %llu already in badblocklist",
> > > + block);
> > > + spin_unlock_irqrestore(&dd->dust_lock, flags);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + bblock = kmalloc(sizeof(*bblock), GFP_KERNEL);
> >
> > You can't do a GFP_KERNEL allocation while holding a spinlock. I think
> > it would be better to assume that the user was correctly adding a new
> > block, do the allocation, and then just call dust_rb_insert() under the
> > spinlock. dust_rb_insert() will return false if the block is already
> > inserted
>
> Ah, okay. I'll be spinning a v2 of this patch. (I need to make some
> updates to the dm-dust.txt documentation file as well, so those
> updates will be in there.)
>
> After reading through your description of the timeline, I created this
> revision of dust_add_block() (which compiles, and seems to work
> properly in my test module):
>
> static int dust_add_block(struct dust_device *dd, unsigned long long block)
> {
> struct badblock *bblock;
> unsigned long flags;
>
> bblock = kmalloc(sizeof(*bblock), GFP_KERNEL);
> if (bblock == NULL) {
> if (!dd->quiet_mode)
> DMERR("addbadblock: badblock allocation failed");
> return -ENOMEM;
> }
>
> spin_lock_irqsave(&dd->dust_lock, flags);
> bblock->bb = block * dd->sect_per_block;
> if (!dust_rb_insert(&dd->badblocklist, bblock)) {
> if (!dd->quiet_mode)
> DMERR("addbadblock: block %llu already in badblocklist",
> block);
> spin_unlock_irqrestore(&dd->dust_lock, flags);
> kfree(bblock);
> return -EINVAL;
> }
>
> dd->badblock_count++;
> if (!dd->quiet_mode)
> DMINFO("addbadblock: badblock added at block %llu", block);
> spin_unlock_irqrestore(&dd->dust_lock, flags);
> return 0;
> }
>
> Test results:
>
> # dmsetup message dust1 0 addbadblock 60
> kernel: device-mapper: dust: addbadblock: badblock added at block 60
> # dmsetup message dust1 0 addbadblock 60
> kernel: device-mapper: dust: addbadblock: block 60 already in badblocklist
> # dmsetup message dust1 0 addbadblock 60
> kernel: device-mapper: dust: addbadblock: block 60 already in badblocklist
>
> The kmalloc() is before the spinlock, and if the block is already in
> the rbtree, it unlocks, and frees the unused "bblock". Does this look
> good?
>
Yep. That looks fine (other than your ">" and "&" characters getting
lost in translation)
-Ben
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
