[dm-devel] [RFC PATCH 1/1] dm: add dust target

[Benjamin Marzinski]

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 at redhat.com> 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