Re: [patch][rfc] 5/5: core remove PageReserved

Re: [patch][rfc] 5/5: core remove PageReserved

[Andrew Morton]

William Lee Irwin III <wli@holomorphy.com> wrote:
>
>  On Thu, Jun 23, 2005 at 05:08:24PM +1000, Nick Piggin wrote:
>  > Index: linux-2.6/drivers/scsi/sg.c
>  > ===================================================================
>  > --- linux-2.6.orig/drivers/scsi/sg.c
>  > +++ linux-2.6/drivers/scsi/sg.c
>  > @@ -1887,9 +1887,10 @@ st_unmap_user_pages(struct scatterlist *
>  >  	int i;
>  >  
>  >  	for (i=0; i < nr_pages; i++) {
>  > -		if (dirtied && !PageReserved(sgl[i].page))
>  > +		if (dirtied)
>  >  			SetPageDirty(sgl[i].page);
>  >  		/* unlock_page(sgl[i].page); */
>  > +		/* FIXME: XXX don't dirty/unmap VM_RESERVED regions? */
>  >  		/* FIXME: cache flush missing for rw==READ
>  >  		 * FIXME: call the correct reference counting function
>  >  		 */
> 
>  An answer should be devised for this. My numerous SCSI CD-ROM devices
>  (I have 5 across several different machines of several different arches)
>  are rather unlikely to be happy with /* FIXME: XXX ... as an answer.
> 
> 
>  On Thu, Jun 23, 2005 at 05:08:24PM +1000, Nick Piggin wrote:
>  > Index: linux-2.6/drivers/scsi/st.c
>  > ===================================================================
>  > --- linux-2.6.orig/drivers/scsi/st.c
>  > +++ linux-2.6/drivers/scsi/st.c
>  > @@ -4435,8 +4435,9 @@ static int sgl_unmap_user_pages(struct s
>  >  	int i;
>  >  
>  >  	for (i=0; i < nr_pages; i++) {
>  > -		if (dirtied && !PageReserved(sgl[i].page))
>  > +		if (dirtied)
>  >  			SetPageDirty(sgl[i].page);
>  > +		/* FIXME: XXX don't dirty/unmap VM_RESERVED regions? */
>  >  		/* FIXME: cache flush missing for rw==READ
>  >  		 * FIXME: call the correct reference counting function
>  >  		 */
> 
>  Mutatis mutandis for my SCSI tape drive.

This scsi code is already rather wrong.  There isn't much point in just
setting PG_dirty and leaving the page marked as clean in the radix tree. 
As it is we'll lose data if the user reads it into a MAP_SHARED memory
buffer.

set_page_dirty_lock() should be used here.  That can sleep.

<looks>

The above two functions are called under write_lock_irqsave() (at least)
and might be called from irq context (dunno).  So we cannot use
set_page_dirty_lock() and we don't have a ref on the page's inode.  We
could use set_page_dirty() and be racy against page reclaim.

But to get all this correct (and it's very incorrect now) we'd need to punt
the page dirtying up to process context, along the lines of
bio_check_pages_dirty().

Or, if st_unmap_user_pages() and sgl_unmap_user_pages() are not called from
irq context then we should arrange for them to be called without locks held
and use set_page_dirty_lock().

Re: [patch][rfc] 5/5: core remove PageReserved

[William Lee Irwin III]

William Lee Irwin III <wli@holomorphy.com> wrote:
>>  An answer should be devised for this. My numerous SCSI CD-ROM devices
>>  (I have 5 across several different machines of several different arches)
>>  are rather unlikely to be happy with /* FIXME: XXX ... as an answer.
[...]
>>  Mutatis mutandis for my SCSI tape drive.

On Thu, Jun 23, 2005 at 09:50:11PM -0700, Andrew Morton wrote:
> This scsi code is already rather wrong.  There isn't much point in just
> setting PG_dirty and leaving the page marked as clean in the radix tree. 
> As it is we'll lose data if the user reads it into a MAP_SHARED memory
> buffer.
> set_page_dirty_lock() should be used here.  That can sleep.
> The above two functions are called under write_lock_irqsave() (at least)
> and might be called from irq context (dunno).  So we cannot use
> set_page_dirty_lock() and we don't have a ref on the page's inode.  We
> could use set_page_dirty() and be racy against page reclaim.
> But to get all this correct (and it's very incorrect now) we'd need to punt
> the page dirtying up to process context, along the lines of
> bio_check_pages_dirty().
> Or, if st_unmap_user_pages() and sgl_unmap_user_pages() are not called from
> irq context then we should arrange for them to be called without locks held
> and use set_page_dirty_lock().

This all sounds very reasonable. I was originally more concerned about
the new FIXME getting introduced but this sounds like a good way to
resolve the preexisting FIXME's surrounding all this.


-- wli
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [patch][rfc] 5/5: core remove PageReserved

[Nick Piggin]

Andrew Morton wrote:
> William Lee Irwin III <wli@holomorphy.com> wrote:

>> Mutatis mutandis for my SCSI tape drive.
> 
> 

OK, for the VM_RESERVED case, it looks like it won't be much of a problem
because get_user_pages faults on VM_IO regions (which is already set in
remap_pfn_range which is used by mem.c and most drivers). So this code will
simply not encounter VM_RESERVED regions - well obviously, get_user_pages
should be made to explicitly check for VM_RESERVED too, but the point being
that introducing such a check will not overly restrict drivers.

[snip SetPageDirty is wrong]

Not that this helps the existing bug...

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com