Messages in this thread Patch in this message | | | Date | Mon, 9 Feb 2009 19:48:25 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH] Propagate CRAMFS uncompression errors |
| |
On Mon, 9 Feb 2009 22:28:15 -0500 "David VomLehn (dvomlehn)" <dvomlehn@cisco.com> wrote:
> > > From: Andrew Morton [mailto:akpm@linux-foundation.org] > > Sent: Monday, February 09, 2009 3:17 PM > > Subject: Re: [PATCH] Propagate CRAMFS uncompression errors > > > > "David VomLehn (dvomlehn)" <dvomlehn@cisco.com> wrote: > > > > > If cramfs_uncompress_block detects an error uncompressing it will > > > return a zero value. This patch checks the return value and > > propagates > > > the error back up to the block layer. > > > > > > Signed-off-by: David VomLehn <dvomlehn@cisco.com> > > > --- > > > fs/cramfs/inode.c | 10 +++++++++- > > > 1 files changed, 9 insertions(+), 1 deletions(-) > > > > > > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c > > > index a07338d..6ff8a5e 100644 > > > --- a/fs/cramfs/inode.c > > > +++ b/fs/cramfs/inode.c > > > @@ -493,7 +493,15 @@ static int cramfs_readpage(struct file *file, > > > struct page * page) > > > > Your email client is wordwrapping the patches. > > Sorry. I sent it to myself first, and didn't have a problem. I hate > email programs. > > > > memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - > > > bytes_filled); > > > kunmap(page); > > > flush_dcache_page(page); > > > - SetPageUptodate(page); > > > + > > > + if (bytes_filled == 0) { > > > + ClearPageUptodate(page); > > > + SetPageError(page); > > > + } > > > + > > > + else > > > + SetPageUptodate(page); > > > + > > > unlock_page(page); > > > return 0; > > > > A more typical code layout would be > > > > if (bytes_filled == 0) { > > ClearPageUptodate(page); > > SetPageError(page); > > } else > > SetPageUptodate(page); > > > > or (better, IMO): > > > > if (bytes_filled == 0) { > > ClearPageUptodate(page); > > SetPageError(page); > > } else { > > SetPageUptodate(page); > > } > > > > > > This patch will incorrectly cause the driver to report an IO error if > > the (page->index < maxblock) test returns false. For example, a > > pread() which is wholly outside the end-of-file should return > > zero, not > > -EIO. > > > > cramfs_readpage() handles this case very strangely, although not > > obviously buggily. Probably this function never even gets > > called for a > > read wholly outside i_size. > > > > How does this version look to you? > > > > --- a/fs/cramfs/inode.c~propagate-cramfs-uncompression-errors > > +++ a/fs/cramfs/inode.c > > @@ -488,12 +488,19 @@ static int cramfs_readpage(struct file * > > compr_len); > > mutex_unlock(&read_mutex); > > } > > - } else > > - pgdata = kmap(page); > > - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - > > bytes_filled); > > - kunmap(page); > > - flush_dcache_page(page); > > - SetPageUptodate(page); > > + > > + if (bytes_filled == 0) { > > + /* Decompression error */ > > + ClearPageUptodate(page); > > + SetPageError(page); > > + } else { > > + memset(pgdata + bytes_filled, 0, > > + PAGE_CACHE_SIZE - bytes_filled); > > + flush_dcache_page(page); > > + SetPageUptodate(page); > > + } > > + kunmap(page); > > + } > > unlock_page(page); > > return 0; > > } > > Better, thanks!
umm...
Nope, it's still not right. We'll treat this case:
if (compr_len == 0) ; /* hole */
as an IO error. grr.
--- a/fs/cramfs/inode.c~cramfs-propagate-uncompression-errors +++ a/fs/cramfs/inode.c @@ -477,7 +477,7 @@ static int cramfs_readpage(struct file * mutex_unlock(&read_mutex); pgdata = kmap(page); if (compr_len == 0) - ; /* hole */ + goto out; /* hole */ else if (compr_len > (PAGE_CACHE_SIZE << 1)) printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", compr_len); else { @@ -488,12 +488,20 @@ static int cramfs_readpage(struct file * compr_len); mutex_unlock(&read_mutex); } - } else - pgdata = kmap(page); - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled); - kunmap(page); - flush_dcache_page(page); - SetPageUptodate(page); + + if (bytes_filled == 0) { + /* Decompression error */ + ClearPageUptodate(page); + SetPageError(page); + } else { + memset(pgdata + bytes_filled, 0, + PAGE_CACHE_SIZE - bytes_filled); + flush_dcache_page(page); + SetPageUptodate(page); + } + kunmap(page); + } +out: unlock_page(page); return 0; } _
| |