Skip to contentSkip to conversation

Unnecessary barrier in sync_page()?

DISCUSSION Marcelo Tosatti 4 participants 14 messages Jul 7, 2004

lore
Share & tools
lore

Marcelo Tosatti

author
#0 lore raw

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

#1 lore raw
Quoted 51 lines · 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);
}
Quoted 1 line

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.

#2 lore raw
Quoted 8 lines · Andrea Arcangeli

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().

#3 lore raw
Quoted 7 lines · Andrew Morton
Quoted 6 lines · Andrea Arcangeli

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.

Quoted 5 lines
Quoted 2 lines

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?

#4 lore raw
Quoted 8 lines · Andrea Arcangeli
Quoted 7 lines · Andrew Morton
Quoted 6 lines · Andrea Arcangeli

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.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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.

#5 lore raw
In reply to Andrea Arcangeli
Quoted 8 lines · Andrea Arcangeli
Quoted 7 lines · Andrew Morton
Quoted 6 lines · Andrea Arcangeli

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.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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

#6 lore raw
Quoted 2 lines · Chris Mason

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.

#7 lore raw
Quoted 6 lines · Andrea Arcangeli
Quoted 2 lines · Chris Mason

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.

Quoted 8 lines

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

#8 lore raw
In reply to Andrea Arcangeli
Quoted 9 lines · Andrea Arcangeli
Quoted 2 lines · Chris Mason

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.

Quoted 4 lines

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...

#9 lore raw
Quoted 2 lines · Andrew Morton

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

#10 lore raw
In reply to Andrew Morton
Quoted 2 lines · Andrew Morton

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

exactly ;)

Quoted 2 lines

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?

#11 lore raw
Quoted 5 lines · Andrea Arcangeli

'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.

author
#12 lore raw
In reply to Andrea Arcangeli
Quoted 52 lines · Andrea Arcangeli
Quoted 51 lines · 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);
}
Quoted 5 lines
Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 2 lines

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." ?

#13 lore raw

Hi Marcelo,

Quoted 10 lines · Marcelo Tosatti

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.

Quoted 4 lines

"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).

Quoted 5 lines

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.