Re: [md PATCH 01/36] md: beginnings of bad block management.

[Namhyung Kim <namhyung@gmail.com>]

2011-07-26 (화), 12:26 +1000, NeilBrown:
> On Sat, 23 Jul 2011 00:03:45 +0900 Namhyung Kim <namhyung@gmail.com> wrote:
> 
> > NeilBrown <neilb@suse.de> writes:
> > 
> > > This the first step in allowing md to track bad-blocks per-device so
> > > that we can fail individual blocks rather than the whole device.
> > >
> > > This patch just adds a data structure for recording bad blocks, with
> > > routines to add, remove, search the list.
> > >
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > >
> > >  drivers/md/md.c |  457 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/md/md.h |   49 ++++++
> > >  2 files changed, 502 insertions(+), 4 deletions(-)
> > >
> > > +
> > > +/* Bad block management.
> > > + * We can record which blocks on each device are 'bad' and so just
> > > + * fail those blocks, or that stripe, rather than the whole device.
> > > + * Entries in the bad-block table are 64bits wide.  This comprises:
> > > + * Length of bad-range, in sectors: 0-511 for lengths 1-512
> > > + * Start of bad-range, sector offset, 54 bits (allows 8 exbibytes)
> > > + *  A 'shift' can be set so that larger blocks are tracked and
> > > + *  consequently larger devices can be covered.
> > > + * 'Acknowledged' flag - 1 bit. - the most significant bit.
> > > + */
> > > +/* Locking of the bad-block table is a two-layer affair.
> > > + * Read access through ->active_page only requires an rcu_readlock.
> > > + * However if ->active_page is found to be NULL, the table
> > > + * should be accessed through ->page which requires an irq-spinlock.
> > > + * Updating the page requires setting ->active_page to NULL,
> > > + * synchronising with rcu, then updating ->page under the same
> > > + * irq-spinlock.
> > > + * We always set or clear bad blocks from process context, but
> > > + * might look-up bad blocks from interrupt/bh context.
> > > + *
> > 
> > Empty line.
> > 
> > If the locking is complex, it'd be better defining separate functions to
> > deal with it, IMHO. Please see below.
> 
> I too have been feeling uncomfortable about the locking and I recently
> realised that I really should be using a seqlock rather than trying to force
> RCU into the mould.  So I have changed it and it is much better now.  Below
> is new version.
> 
> 
> > > +EXPORT_SYMBOL_GPL(rdev_set_badblocks);
> > 
> > I think it would be better if all exported functions in md.c have
> > prefixed 'md_'.
> > 
> 
> Probably good advice.  I don't think I'll change it now but maybe in a
> subsequent patch so that I change them all at once.
> 
> Thanks,
> NeilBrown
> 
> commit 0980048be17a45ae9e181ad04a549c31a499dee9
> Author: NeilBrown <neilb@suse.de>
> Date:   Tue Jul 26 12:22:08 2011 +1000
> 
>     md: beginnings of bad block management.
>     
>     This the first step in allowing md to track bad-blocks per-device so
>     that we can fail individual blocks rather than the whole device.
>     
>     This patch just adds a data structure for recording bad blocks, with
>     routines to add, remove, search the list.
>     
>     Signed-off-by: NeilBrown <neilb@suse.de>

with your another badblocks.page fix:

  Reviewed-by: Namhyung Kim <namhyung@gmail.com>


-- 
Regards,
Namhyung Kim


--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html