Unnecessary barrier in sync_page()?

Unnecessary barrier in sync_page()?

[Marcelo Tosatti]

Hi Chris,

I was talking to Andrew about this memory barrier 

static inline int sync_page(struct page *page)
{
        struct address_space *mapping;
                                                                                        
        /*
         * FIXME, fercrissake.  What is this barrier here for?
         */
        smp_mb();
        mapping = page_mapping(page);
        if (mapping && mapping->a_ops && mapping->a_ops->sync_page)
                return mapping->a_ops->sync_page(page);
        return 0;
}

And does not seem to be a reason for it. The callers are:

void fastcall wait_on_page_bit(struct page *page, int bit_nr)
{
        wait_queue_head_t *waitqueue = page_waitqueue(page);
        DEFINE_PAGE_WAIT(wait, page, bit_nr);
                                                                                        
        do {
                prepare_to_wait(waitqueue, &wait.wait, TASK_UNINTERRUPTIBLE);
                if (test_bit(bit_nr, &page->flags)) {
                        sync_page(page);
                        io_schedule();
                }
        } while (test_bit(bit_nr, &page->flags));
        finish_wait(waitqueue, &wait.wait);
} 

void fastcall __lock_page(struct page *page)
{
        wait_queue_head_t *wqh = page_waitqueue(page);
        DEFINE_PAGE_WAIT_EXCLUSIVE(wait, page, PG_locked);
                                                                                        
        while (TestSetPageLocked(page)) {
                prepare_to_wait_exclusive(wqh, &wait.wait, TASK_UNINTERRUPTIBLE);
                if (PageLocked(page)) {
                        sync_page(page);
                        io_schedule();
                }
        }
        finish_wait(wqh, &wait.wait);
}

Both callers call set_bit (atomic operation which cannot be reordered) before 
sync_page(), so.. what is the barrier trying to guarantee? 

TIA

Re: Unnecessary barrier in sync_page()?

[Andrea Arcangeli]

On Wed, Jul 07, 2004 at 02:57:24PM -0300, Marcelo Tosatti wrote:
> 
> Hi Chris,
> 
> I was talking to Andrew about this memory barrier 
> 
> static inline int sync_page(struct page *page)
> {
>         struct address_space *mapping;
>                                                                                         
>         /*
>          * FIXME, fercrissake.  What is this barrier here for?
>          */
>         smp_mb();
>         mapping = page_mapping(page);
>         if (mapping && mapping->a_ops && mapping->a_ops->sync_page)
>                 return mapping->a_ops->sync_page(page);
>         return 0;
> }
> 
> And does not seem to be a reason for it. The callers are:
> 
> void fastcall wait_on_page_bit(struct page *page, int bit_nr)
> {
>         wait_queue_head_t *waitqueue = page_waitqueue(page);
>         DEFINE_PAGE_WAIT(wait, page, bit_nr);
>                                                                                         
>         do {
>                 prepare_to_wait(waitqueue, &wait.wait, TASK_UNINTERRUPTIBLE);
>                 if (test_bit(bit_nr, &page->flags)) {
>                         sync_page(page);
>                         io_schedule();
>                 }
>         } while (test_bit(bit_nr, &page->flags));
>         finish_wait(waitqueue, &wait.wait);
> } 
> 
> void fastcall __lock_page(struct page *page)
> {
>         wait_queue_head_t *wqh = page_waitqueue(page);
>         DEFINE_PAGE_WAIT_EXCLUSIVE(wait, page, PG_locked);
>                                                                                         
>         while (TestSetPageLocked(page)) {
>                 prepare_to_wait_exclusive(wqh, &wait.wait, TASK_UNINTERRUPTIBLE);
>                 if (PageLocked(page)) {
>                         sync_page(page);
>                         io_schedule();
>                 }
>         }
>         finish_wait(wqh, &wait.wait);
> }
> 
> Both callers call set_bit (atomic operation which cannot be reordered) before 

set_bit is atomic but it _can_ be reordered just fine. atomic !=
barrier (they're the same only in x86 due the lack of specific smp-aware
opcodes).

however the smp_mb() isn't needed in sync_page, simply because it's
perfectly ok if we start running sync_page before reading pagelocked.
All we care about is to run sync_page _before_ io_schedule() and that we
read PageLocked _after_ prepare_to_wait_exclusive.

So the locking in between PageLocked and sync_page is _absolutely_
worthless and the smp_mb() can go away.

Re: Unnecessary barrier in sync_page()?

[Andrew Morton]

Andrea Arcangeli <andrea@suse.de> wrote:
>
> however the smp_mb() isn't needed in sync_page, simply because it's
>  perfectly ok if we start running sync_page before reading pagelocked.
>  All we care about is to run sync_page _before_ io_schedule() and that we
>  read PageLocked _after_ prepare_to_wait_exclusive.
> 
>  So the locking in between PageLocked and sync_page is _absolutely_
>  worthless and the smp_mb() can go away.

IIRC, Chris added that barrier (and several similar) for the reads in
page_mapping().

Re: Unnecessary barrier in sync_page()?

[Andrea Arcangeli]

On Wed, Jul 07, 2004 at 11:29:53AM -0700, Andrew Morton wrote:
> Andrea Arcangeli <andrea@suse.de> wrote:
> >
> > however the smp_mb() isn't needed in sync_page, simply because it's
> >  perfectly ok if we start running sync_page before reading pagelocked.
> >  All we care about is to run sync_page _before_ io_schedule() and that we
> >  read PageLocked _after_ prepare_to_wait_exclusive.
> > 
> >  So the locking in between PageLocked and sync_page is _absolutely_
> >  worthless and the smp_mb() can go away.
> 
> IIRC, Chris added that barrier (and several similar) for the reads in
> page_mapping().

how does it help to know the page is not locked before executing
page_mapped?

Re: Unnecessary barrier in sync_page()?

[Andrea Arcangeli]

On Wed, Jul 07, 2004 at 08:42:02PM +0200, Andrea Arcangeli wrote:
> On Wed, Jul 07, 2004 at 11:29:53AM -0700, Andrew Morton wrote:
> > Andrea Arcangeli <andrea@suse.de> wrote:
> > >
> > > however the smp_mb() isn't needed in sync_page, simply because it's
> > >  perfectly ok if we start running sync_page before reading pagelocked.
> > >  All we care about is to run sync_page _before_ io_schedule() and that we
> > >  read PageLocked _after_ prepare_to_wait_exclusive.
> > > 
> > >  So the locking in between PageLocked and sync_page is _absolutely_
> > >  worthless and the smp_mb() can go away.
> > 
> > IIRC, Chris added that barrier (and several similar) for the reads in
> > page_mapping().
> 
> how does it help to know the page is not locked before executing
> page_mapped?

(I expressed it a bit bad, I mean not locked by us)

I mean the lock on the page can go from under us anytime. the the very
best you can try to hide a race condition with such smp_mb, but sure not
fix it.

Re: Unnecessary barrier in sync_page()?

[Marcelo Tosatti]

On Wed, Jul 07, 2004 at 08:20:25PM +0200, Andrea Arcangeli wrote:
> On Wed, Jul 07, 2004 at 02:57:24PM -0300, Marcelo Tosatti wrote:
> > 
> > Hi Chris,
> > 
> > I was talking to Andrew about this memory barrier 
> > 
> > static inline int sync_page(struct page *page)
> > {
> >         struct address_space *mapping;
> >                                                                                         
> >         /*
> >          * FIXME, fercrissake.  What is this barrier here for?
> >          */
> >         smp_mb();
> >         mapping = page_mapping(page);
> >         if (mapping && mapping->a_ops && mapping->a_ops->sync_page)
> >                 return mapping->a_ops->sync_page(page);
> >         return 0;
> > }
> > 
> > And does not seem to be a reason for it. The callers are:
> > 
> > void fastcall wait_on_page_bit(struct page *page, int bit_nr)
> > {
> >         wait_queue_head_t *waitqueue = page_waitqueue(page);
> >         DEFINE_PAGE_WAIT(wait, page, bit_nr);
> >                                                                                         
> >         do {
> >                 prepare_to_wait(waitqueue, &wait.wait, TASK_UNINTERRUPTIBLE);
> >                 if (test_bit(bit_nr, &page->flags)) {
> >                         sync_page(page);
> >                         io_schedule();
> >                 }
> >         } while (test_bit(bit_nr, &page->flags));
> >         finish_wait(waitqueue, &wait.wait);
> > } 
> > 
> > void fastcall __lock_page(struct page *page)
> > {
> >         wait_queue_head_t *wqh = page_waitqueue(page);
> >         DEFINE_PAGE_WAIT_EXCLUSIVE(wait, page, PG_locked);
> >                                                                                         
> >         while (TestSetPageLocked(page)) {
> >                 prepare_to_wait_exclusive(wqh, &wait.wait, TASK_UNINTERRUPTIBLE);
> >                 if (PageLocked(page)) {
> >                         sync_page(page);
> >                         io_schedule();
> >                 }
> >         }
> >         finish_wait(wqh, &wait.wait);
> > }
> > 
> > Both callers call set_bit (atomic operation which cannot be reordered) before 
> 
> set_bit is atomic but it _can_ be reordered just fine. atomic !=
> barrier (they're the same only in x86 due the lack of specific smp-aware
> opcodes).

Hi Andrea,

OK, got it.

I think this comment on i386 bitops.h can lead to 
misunderstanding and should be changed:

/**
 * set_bit - Atomically set a bit in memory
 * @nr: the bit to set
 * @addr: the address to start counting from
 *
 * This function is atomic and may not be reordered. 

"This function is atomic and may not be reordered (other architectures MAY reorder it)"

Or something like this. The comment as it is leads people to
write nonportable code which assumes set_bit() cant be reordered. Like naive me did.

> however the smp_mb() isn't needed in sync_page, simply because it's
> perfectly ok if we start running sync_page before reading pagelocked.
>
> All we care about is to run sync_page _before_ io_schedule() and that we
> read PageLocked _after_ prepare_to_wait_exclusive.

Ok, I see, yes.

> So the locking in between PageLocked and sync_page is _absolutely_
> worthless and the smp_mb() can go away. 

Andrew, what you think about removing that barrier from sync_page() 
on -mm? 

And what about changing the "may not reordered" comments on i386 bitops.h
to "may not be reordered on i386 only, other arches do reorder it." ?

Re: Unnecessary barrier in sync_page()?

[Andrea Arcangeli]

Hi Marcelo,

On Wed, Jul 07, 2004 at 03:58:14PM -0300, Marcelo Tosatti wrote:
> I think this comment on i386 bitops.h can lead to 
> misunderstanding and should be changed:
> 
> /**
>  * set_bit - Atomically set a bit in memory
>  * @nr: the bit to set
>  * @addr: the address to start counting from
>  *
>  * This function is atomic and may not be reordered. 
> 
> "This function is atomic and may not be reordered (other architectures MAY reorder it)"
> 
> Or something like this. The comment as it is leads people to
> write nonportable code which assumes set_bit() cant be reordered. Like naive me did.

agreed. (as usual trusting comments more than code carries some
risk, last time I was fooled by a comment was only a few months ago too
in some device driver ;)

btw, for completeness, test_bit (the one running before sync_page) can
be reordered even in x86.

The only one that enforces ordering everywhere is test_and_set_bit (as
Linus recently reminded on the list too). And it only enforces ordering
if it returns 0. If it returns 1 no ordering is enforced (for example on
alpha) because no change was made to the memory and supposedly the code
will not be allowed to access any critical section (and in turn no need
of any barrier).

> Andrew, what you think about removing that barrier from sync_page() 
> on -mm? 
> 
> And what about changing the "may not reordered" comments on i386 bitops.h
> to "may not be reordered on i386 only, other arches do reorder it." ?

both looks correct to me, thanks.

Re: Unnecessary barrier in sync_page()?

[Chris Mason]

On Wed, 2004-07-07 at 14:42, Andrea Arcangeli wrote:
> On Wed, Jul 07, 2004 at 11:29:53AM -0700, Andrew Morton wrote:
> > Andrea Arcangeli <andrea@suse.de> wrote:
> > >
> > > however the smp_mb() isn't needed in sync_page, simply because it's
> > >  perfectly ok if we start running sync_page before reading pagelocked.
> > >  All we care about is to run sync_page _before_ io_schedule() and that we
> > >  read PageLocked _after_ prepare_to_wait_exclusive.
> > > 
> > >  So the locking in between PageLocked and sync_page is _absolutely_
> > >  worthless and the smp_mb() can go away.
> > 
> > IIRC, Chris added that barrier (and several similar) for the reads in
> > page_mapping().
> 
> how does it help to know the page is not locked before executing
> page_mapped?

I wasn't worried about the locked bit when I added the barrier, my goal
was to order things with people that set page->mapping to null.

-chris

Re: Unnecessary barrier in sync_page()?

[Andrea Arcangeli]

On Wed, Jul 07, 2004 at 04:57:04PM -0400, Chris Mason wrote:
> I wasn't worried about the locked bit when I added the barrier, my goal
> was to order things with people that set page->mapping to null.

page->mapping cannot change from NULL to non-NULL there.

it can only change from non-NULL to NULL, and there's no way to
serialize with the truncate without taking the page lock.

The one extremely important fix you did around the same time, has been
to "cache" the value of "mapping" in the kernel stack, so that it
remains the same during the while function (so that it cannot start
non-NULL an finish NULL). But the smp_mb() itself cannot make a
difference as far as I can tell.

Re: Unnecessary barrier in sync_page()?

[Chris Mason]

On Wed, 2004-07-07 at 17:06, Andrea Arcangeli wrote:
> On Wed, Jul 07, 2004 at 04:57:04PM -0400, Chris Mason wrote:
> > I wasn't worried about the locked bit when I added the barrier, my goal
> > was to order things with people that set page->mapping to null.
> 
> page->mapping cannot change from NULL to non-NULL there.
> 
> it can only change from non-NULL to NULL, and there's no way to
> serialize with the truncate without taking the page lock.
> 
> The one extremely important fix you did around the same time, has been
> to "cache" the value of "mapping" in the kernel stack, so that it
> remains the same during the while function (so that it cannot start
> non-NULL an finish NULL). But the smp_mb() itself cannot make a
> difference as far as I can tell.

As Andrew pointed out back then, page->mapping can go to null, but even
if we have a stale copy of page->mapping, the mapping can't be freed. 
So you're right that it should be enough to keep the change to cache the
value of mapping.  

I was hunting the backing dev info bugs back then, and seem to have
talked myself into the barriers while testing...

-chris

Re: Unnecessary barrier in sync_page()?

[Andrew Morton]

Andrea Arcangeli <andrea@suse.de> wrote:
>
> On Wed, Jul 07, 2004 at 04:57:04PM -0400, Chris Mason wrote:
> > I wasn't worried about the locked bit when I added the barrier, my goal
> > was to order things with people that set page->mapping to null.
> 
> page->mapping cannot change from NULL to non-NULL there.
> 
> it can only change from non-NULL to NULL, and there's no way to
> serialize with the truncate without taking the page lock.

And we cannot lock the page because, err, we need to run sync_page() for
that.

> The one extremely important fix you did around the same time, has been
> to "cache" the value of "mapping" in the kernel stack, so that it
> remains the same during the while function (so that it cannot start
> non-NULL an finish NULL).

But the page can come unlocked and truncate or page reclaim can remove the
page from the mapping and memory reclaim can reclaim the inode:

	int block_sync_page(struct page *page)
	{
		struct address_space *mapping;

		smp_mb();
		mapping = page_mapping(page);
-> right here
		if (mapping)
-> go boom here		blk_run_backing_dev(mapping->backing_dev_info, page);
		return 0;
	}

But I cannot think of any callers of sync_page() who don't have a ref on
the inode, so...

Re: Unnecessary barrier in sync_page()?

[Chris Mason]

On Wed, 2004-07-07 at 17:30, Andrew Morton wrote:

> But I cannot think of any callers of sync_page() who don't have a ref on
> the inode, so...

I looked pretty hard for callers that didn't have a ref on the inode
while debugging the backing dev oopsen...I couldn't find any.

-chris

Re: Unnecessary barrier in sync_page()?

[Andrea Arcangeli]

On Wed, Jul 07, 2004 at 02:30:15PM -0700, Andrew Morton wrote:
> And we cannot lock the page because, err, we need to run sync_page() for
> that.

exactly ;)

> But I cannot think of any callers of sync_page() who don't have a ref on
> the inode, so...

I'm thinking, does handle_write_error() holds a ref on the inode? that's
the VM and it finds the page without passing through the inode. I'm
afraid the VM isn't safe calling lock_page, or am I overlooking
something here?

Re: Unnecessary barrier in sync_page()?

[Andrew Morton]

Andrea Arcangeli <andrea@suse.de> wrote:
>
> 'm thinking, does handle_write_error() holds a ref on the inode? that's
> the VM and it finds the page without passing through the inode. I'm
> afraid the VM isn't safe calling lock_page, or am I overlooking
> something here?

Yes, that's buggy - the caller has a ref on the page, but not on the inode.

I'm not sure what's worth doing in there - maybe just trylock it and if
that fails, lose the I/O error.  Combine that with propagation of the page
error flags into the address_space within truncate_complete_page() and
invalidate_complete_page() and in shrink_list().  Does that cover
everything?

Or just kill handle_write_error() altogether and push the responsibility
for setting the address_space error bits into writepage() -
block_write_full_page() and mpage_writepage() do it already.

Let me think about it a bit.