On Tue, Mar 04, 2008 at 01:19:57PM +0100, Guilhem Bichot wrote:
> Hello,
>
> On Tue, Mar 04, 2008 at 01:58:56PM +0200, sanja@stripped wrote:
> > ChangeSet@stripped, 2008-03-04 13:58:50+02:00, bell@stripped +1 -0
> > Fixed problem of deleting blocks which are being evicted at
> > the moment. (BUG#34634)
> > Fixed potential bug in pinning schema.
> >
> > storage/maria/ma_pagecache.c@stripped, 2008-03-04 13:58:47+02:00,
> bell@stripped +58 -37
> > Fixed problem of deleting blocks which are being evicted at
> > the moment.
> > Fixed potential bug in pinning schema.
> >
> > diff -Nrup a/storage/maria/ma_pagecache.c b/storage/maria/ma_pagecache.c
> > --- a/storage/maria/ma_pagecache.c 2008-02-22 23:16:21 +02:00
> > +++ b/storage/maria/ma_pagecache.c 2008-03-04 13:58:47 +02:00
> > @@ -2440,26 +2441,25 @@ static void release_rdlock(PAGECACHE_BLO
> > DBUG_VOID_RETURN;
> > }
> >
> > -/*
> > - Try to lock/unlock and pin/unpin the block
> > +/**
> > + @brief Try to lock/unlock and pin/unpin the block
> >
> > - SYNOPSIS
> > - make_lock_and_pin()
> > - pagecache pointer to a page cache data structure
> > - block the block to work with
> > - lock lock change mode
> > - pin pinchange mode
> > - file File handler requesting pin
> > + @param pagecache pointer to a page cache data structure
> > + @param block the block to work with
> > + @param lock lock change mode
> > + @param pin pinchange mode
> > + @patam file File handler requesting pin
> > + @param req_registered True if request was registered for the block
>
> And then we need the comment to say that in case of error, the request
> will be automatically un-registered.
the comment will not be needed because I'll follow your other suggestion
about removing the argument.
> > - RETURN
> > - 0 - OK
> > - 1 - Try to lock the block failed
> > + @retval 0 OK
> > + @retval 1 Try to lock the block failed
> > */
> >
> > static my_bool make_lock_and_pin(PAGECACHE *pagecache,
> > PAGECACHE_BLOCK_LINK *block,
> > enum pagecache_page_lock lock,
> > - enum pagecache_page_pin pin)
> > + enum pagecache_page_pin pin,
> > + my_bool req_registered)
> > {
> > DBUG_ENTER("make_lock_and_pin");
> >
> > @@ -2552,7 +2552,8 @@ retry:
>
> That old "retry" name could be changed: there is no retrying in fact,
> it just returns 1 as error. If it was a real retry-ing, the patch
> would be wrong as it would un-register several times (assuming we
> would come to "retry" several times).
It is nquite for retry (upper level should retry (from here it is
name)).
>
> > DBUG_ASSERT(block->hash_link->requests > 0);
> > block->hash_link->requests--;
> > DBUG_ASSERT(block->requests > 0);
> > - unreg_request(pagecache, block, 1);
> > + if (req_registered)
> > + unreg_request(pagecache, block, 1);
> > PCBLOCK_INFO(block);
> > DBUG_RETURN(1);
>
> I wonder:
> in your patch, make_lock_and_pin() is mostly used with req_registered
> a constant 0, except in two places.
> Isn't it more symmetric to have the caller clean up what the caller
> has done (requests), instead of having the situation in the patch
> where:
> - caller registers request
> - and tells make_lock_and_pin() to unregister it in case of failure
> (i.e. caller saying "clean up after me if you fail")?
>
> That would give maybe a more localized fix.
OK, I fix the patch.
> > @@ -2795,7 +2796,7 @@ void pagecache_unlock(PAGECACHE *pagecac
> > (ulong) block));
> > }
> >
> > - if (make_lock_and_pin(pagecache, block, lock, pin))
> > + if (make_lock_and_pin(pagecache, block, lock, pin, 0))
> > {
> > DBUG_ASSERT(0); /* should not happend */
> > }
>
> Are there no cases where it can happen?
> Like: make_lock_and_pin() calling get_wrlock() calling
> translog_wait_lock() failing due to some IN_SWITCH/etc condition.
> If that is possible, we should replace DBUG_ASSERT(0) with returning
> an error.
If we downgrade lock or/and pin we can't wait or retry.
And the same for all other cases except delete by link.
For delete by link we keep the block pinned for sure, so it can't be
evicted (which is the cause of lock failure) so we are still on the safe
side.
[skip]
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Mr. Oleksandr Byelkin <sanja@stripped>
/ /|_/ / // /\ \/ /_/ / /__ MySQL AB, Full-Time Developer
/_/ /_/\_, /___/\___\_\___/ Lugansk, Ukraine
<___/ www.mysql.com
