Re: [PATCH] Cachefiles, mpage_cleardirty addtition, west bridge related

Re: [PATCH] Cachefiles, mpage_cleardirty addtition, west bridge related

[David Cross]

This patch adds the mpage_cleardirty function to the cachefiles implementation in the kernel.
The purpose behind this patch is to allow for file based DMA through an external DMA engine
without the data ever going through the process. The procedure for the usage of this function 
is as follows:
1) User space allocates and maps a file
2) external DMA device transfers the data to non-volatile storage directly without it going through 
   the processor
3) the "dirty" pages must be cleared and invalidated as they do not contain correct information.

I believe that David is the correct maintainer and am hoping that he is willing to ACK this change. Please let me know 
if there are any issues or concerns with this patch or if I should be
asking a different maintainer to ack.
Thanks,
David

Signed-off-by: David Cross <david.cross@cypress.com>

diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/fs/mpage.c linux-next-incl-sdk/fs/mpage.c
--- linux-next-vanilla/fs/mpage.c	2010-08-31 19:32:51.000000000 -0700
+++ linux-next-incl-sdk/fs/mpage.c	2010-09-07 11:52:39.000000000 -0700
@@ -716,3 +716,50 @@ int mpage_writepage(struct page *page, g
 	return ret;
 }
 EXPORT_SYMBOL(mpage_writepage);
+
+int mpage_cleardirty(struct address_space *mapping, int num_pages)
+{
+	int ret = 0;
+	int nr_pages;
+	struct pagevec pvec;
+	pgoff_t index = 0;
+	pgoff_t end;
+
+	pagevec_init(&pvec, 0);
+	index = mapping->writeback_index;
+	end = index + num_pages;
+
+	while ((index <= end) &&
+	       (nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
+				PAGECACHE_TAG_DIRTY, min(end - index,
+					(pgoff_t)PAGEVEC_SIZE-1) + 1))) {
+		unsigned i;
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page *page = pvec.pages[i];
+
+			lock_page(page);
+			ret = clear_page_dirty_for_io(page);
+			if (page_has_private(page))
+				do_invalidatepage(page, 0);
+
+			cancel_dirty_page(page, PAGE_CACHE_SIZE);
+
+			remove_from_page_cache(page);
+			ClearPageMappedToDisk(page);
+			page_cache_release(page);	/* pagecache ref */
+			unlock_page(page);
+
+			if (!ret) {
+				printk(KERN_INFO "mpage_cleardirty: "
+				"clear_page_dirty_for_io returned %d\n", ret);
+				return ret;
+			}
+		}
+		pagevec_release(&pvec);
+		cond_resched();
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(mpage_cleardirty);


---------------------------------------------------------------
This message and any attachments may contain Cypress (or its
subsidiaries) confidential information. If it has been received
in error, please advise the sender and immediately delete this
message.
---------------------------------------------------------------

Re: [PATCH] Cachefiles, mpage_cleardirty addtition, west bridge related

[Greg KH]

On Tue, Sep 07, 2010 at 12:33:31PM -0700, David Cross wrote:
> This patch adds the mpage_cleardirty function to the cachefiles implementation in the kernel.
> The purpose behind this patch is to allow for file based DMA through an external DMA engine
> without the data ever going through the process. The procedure for the usage of this function 
> is as follows:
> 1) User space allocates and maps a file
> 2) external DMA device transfers the data to non-volatile storage directly without it going through 
>    the processor
> 3) the "dirty" pages must be cleared and invalidated as they do not contain correct information.
> 
> I believe that David is the correct maintainer and am hoping that he is willing to ACK this change. Please let me know 
> if there are any issues or concerns with this patch or if I should be
> asking a different maintainer to ack.
> Thanks,
> David
> 
> Signed-off-by: David Cross <david.cross@cypress.com>
> 
> diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/fs/mpage.c linux-next-incl-sdk/fs/mpage.c
> --- linux-next-vanilla/fs/mpage.c	2010-08-31 19:32:51.000000000 -0700
> +++ linux-next-incl-sdk/fs/mpage.c	2010-09-07 11:52:39.000000000 -0700
> @@ -716,3 +716,50 @@ int mpage_writepage(struct page *page, g
>  	return ret;
>  }
>  EXPORT_SYMBOL(mpage_writepage);
> +
> +int mpage_cleardirty(struct address_space *mapping, int num_pages)
> +{
> +	int ret = 0;
> +	int nr_pages;
> +	struct pagevec pvec;
> +	pgoff_t index = 0;
> +	pgoff_t end;
> +
> +	pagevec_init(&pvec, 0);
> +	index = mapping->writeback_index;
> +	end = index + num_pages;
> +
> +	while ((index <= end) &&
> +	       (nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
> +				PAGECACHE_TAG_DIRTY, min(end - index,
> +					(pgoff_t)PAGEVEC_SIZE-1) + 1))) {
> +		unsigned i;
> +
> +		for (i = 0; i < nr_pages; i++) {
> +			struct page *page = pvec.pages[i];
> +
> +			lock_page(page);
> +			ret = clear_page_dirty_for_io(page);
> +			if (page_has_private(page))
> +				do_invalidatepage(page, 0);
> +
> +			cancel_dirty_page(page, PAGE_CACHE_SIZE);
> +
> +			remove_from_page_cache(page);
> +			ClearPageMappedToDisk(page);
> +			page_cache_release(page);	/* pagecache ref */
> +			unlock_page(page);
> +
> +			if (!ret) {
> +				printk(KERN_INFO "mpage_cleardirty: "
> +				"clear_page_dirty_for_io returned %d\n", ret);
> +				return ret;
> +			}
> +		}
> +		pagevec_release(&pvec);
> +		cond_resched();
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(mpage_cleardirty);

EXPORT_SYMBOL_GPL()?

thanks,

greg k-h

Re: [PATCH] Cachefiles, mpage_cleardirty addtition, west bridge related

[David Howells]

David Cross <david.cross@cypress.com> wrote:

> This patch adds the mpage_cleardirty function to the cachefiles implementation in the kernel.

Ummm...  How is this used by cachefiles?  Nothing in this patch actually uses
mpage_cleardirty().

David

RE: [PATCH] Cachefiles, mpage_cleardirty addtition, west bridge related

[David Cross]

> From: dhowells@redhat.com [mailto:dhowells@redhat.com] 

> David Cross <david.cross@cypress.com> wrote:

> > This patch adds the mpage_cleardirty function to the cachefiles >
implementation in the kernel.

> Ummm...  How is this used by cachefiles?  Nothing in this patch actually >
uses
> mpage_cleardirty().

This is used by the current west bridge driver in the linux-next tree,
marked as BROKEN at the moment. It is used in order to clear dirty pages
from the page cache for a specific file after it has been allocated. This
allows west bridge to transfer data from USB to storage directly without
requiring that the file data path go through the CPU. The easiest way to
think of this is that it allows for direct file based DMA. Please let me
know if you have any additional questions or concerns on this.
Thanks,
David


---------------------------------------------------------------
This message and any attachments may contain Cypress (or its
subsidiaries) confidential information. If it has been received
in error, please advise the sender and immediately delete this
message.
---------------------------------------------------------------

RE: [PATCH] Cachefiles, mpage_cleardirty addtition, west bridgerelated

[David Cross]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com] 

> On Tue, Sep 07, 2010 at 12:33:31PM -0700, David Cross wrote:
> > This patch adds the mpage_cleardirty function to the cachefiles
implementation in the kernel.
> > The purpose behind this patch is to allow for file based DMA through an
external DMA engine
> > without the data ever going through the process. The procedure for the
usage of this function 
> > is as follows:
> > 1) User space allocates and maps a file
> > 2) external DMA device transfers the data to non-volatile storage
directly without it going through 
> >    the processor
> > 3) the "dirty" pages must be cleared and invalidated as they do not
contain correct information.
> > 
> > I believe that David is the correct maintainer and am hoping that he is
willing to ACK this change. Please let me know 
> > if there are any issues or concerns with this patch or if I should be
> > asking a different maintainer to ack.
> > Thanks,
> > David
> > 
> > Signed-off-by: David Cross <david.cross@cypress.com>
> > 
> > diff -uprN -X linux-next-vanilla/Documentation/dontdiff
linux-next-vanilla/fs/mpage.c linux-next-incl-sdk/fs/mpage.c
> > --- linux-next-vanilla/fs/mpage.c	2010-08-31 19:32:51.000000000 -0700
> > +++ linux-next-incl-sdk/fs/mpage.c	2010-09-07 11:52:39.000000000 -0700
> > @@ -716,3 +716,50 @@ int mpage_writepage(struct page *page, g
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL(mpage_writepage);
> > +
> > +int mpage_cleardirty(struct address_space *mapping, int num_pages)
> > +{
> > +	int ret = 0;
> > +	int nr_pages;
> > +	struct pagevec pvec;
> > +	pgoff_t index = 0;
> > +	pgoff_t end;
> > +
> > +	pagevec_init(&pvec, 0);
> > +	index = mapping->writeback_index;
> > +	end = index + num_pages;
> > +
> > +	while ((index <= end) &&
> > +	       (nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
> > +				PAGECACHE_TAG_DIRTY, min(end - index,
> > +					(pgoff_t)PAGEVEC_SIZE-1) + 1))) {
> > +		unsigned i;
> > +
> > +		for (i = 0; i < nr_pages; i++) {
> > +			struct page *page = pvec.pages[i];
> > +
> > +			lock_page(page);
> > +			ret = clear_page_dirty_for_io(page);
> > +			if (page_has_private(page))
> > +				do_invalidatepage(page, 0);
> > +
> > +			cancel_dirty_page(page, PAGE_CACHE_SIZE);
> > +
> > +			remove_from_page_cache(page);
> > +			ClearPageMappedToDisk(page);
> > +			page_cache_release(page);	/* pagecache ref */
> > +			unlock_page(page);
> > +
> > +			if (!ret) {
> > +				printk(KERN_INFO "mpage_cleardirty: "
> > +				"clear_page_dirty_for_io returned %d\n",
ret);
> > +				return ret;
> > +			}
> > +		}
> > +		pagevec_release(&pvec);
> > +		cond_resched();
> > +	}
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(mpage_cleardirty);

> EXPORT_SYMBOL_GPL()?
Sure, but do I need to submit a new patch for this as well?

thanks,

david


---------------------------------------------------------------
This message and any attachments may contain Cypress (or its
subsidiaries) confidential information. If it has been received
in error, please advise the sender and immediately delete this
message.
---------------------------------------------------------------

Re: [PATCH] Cachefiles, mpage_cleardirty addtition, west bridge related

[Christoph Hellwig]

On Wed, Sep 08, 2010 at 09:51:22AM -0700, David Cross wrote:
> This is used by the current west bridge driver in the linux-next tree,
> marked as BROKEN at the moment. It is used in order to clear dirty pages
> from the page cache for a specific file after it has been allocated. This
> allows west bridge to transfer data from USB to storage directly without
> requiring that the file data path go through the CPU. The easiest way to
> think of this is that it allows for direct file based DMA. Please let me
> know if you have any additional questions or concerns on this.

It's sad enough that it was taken into any tree at all, but we're not
going to add incorrect crap into the kernel kernel for it.

mpage_* are pagecache manpipulation routines, and are only for use by
filesystems for inodes they manage.  A driver may never access this
directly.  Is this the same piece of junk you wanted fat_get_block
exported for?

RE: [PATCH] Cachefiles, mpage_cleardirty addtition, west bridge related

[David Cross]

-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org] 

On Wed, Sep 08, 2010 at 09:51:22AM -0700, David Cross wrote:
> > This is used by the current west bridge driver in the linux-next tree,
> > marked as BROKEN at the moment. It is used in order to clear dirty pages
> > from the page cache for a specific file after it has been allocated.
This
> > allows west bridge to transfer data from USB to storage directly without
> > requiring that the file data path go through the CPU. The easiest way to
> > think of this is that it allows for direct file based DMA. Please let me
> > know if you have any additional questions or concerns on this.

> It's sad enough that it was taken into any tree at all, but we're not
> going to add incorrect crap into the kernel kernel for it.

Can you elaborate on what you mean by "incorrect crap" in a constructive way
if at all possible please?

> mpage_* are pagecache manpipulation routines, and are only for use by
> filesystems for inodes they manage.  A driver may never access this
> directly.

Then what about a user space call such as falloc()? Is your objection to the
existence of the code or the place where it is called from?

> Is this the same piece of junk you wanted fat_get_block
> exported for?

It is the same driver code, but I removed the fat_get_block call in a patch
that I submitted previously based on past inputs, instead adding a few lines
to this function.





---------------------------------------------------------------
This message and any attachments may contain Cypress (or its
subsidiaries) confidential information. If it has been received
in error, please advise the sender and immediately delete this
message.
---------------------------------------------------------------