On Wed, 19 Oct 2011 14:01:48 +0400
Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2011/10/11 Jeff Layton <jlayton@xxxxxxxxxx>:
> ...
> > +
> > +static void
> > +cifs_readv_complete(struct work_struct *work)
> > +{
> > + struct cifs_readdata *rdata = container_of(work,
> > + struct cifs_readdata, work);
> > + struct page *page, *tpage;
> > +
> > + list_for_each_entry_safe(page, tpage, &rdata->pages, lru) {
> > + list_del(&page->lru);
> > + lru_cache_add_file(page);
> > +
> > + if (rdata->result == 0) {
> > + kunmap(page);
> > + flush_dcache_page(page);
> > + SetPageUptodate(page);
> > + unlock_page(page);
> > + cifs_readpage_to_fscache(rdata->mapping->host, page);
> > + }
>
> It seems that we should to kunmap and unlock_page for error case too.
> This change works to me (applied on top of this patch):
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 99ab419..a0e23d6 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1619,15 +1619,18 @@ cifs_readv_complete(struct work_struct *work)
> list_for_each_entry_safe(page, tpage, &rdata->pages, lru) {
> list_del(&page->lru);
> lru_cache_add_file(page);
> + kunmap(page);
>
> if (rdata->result == 0) {
> - kunmap(page);
> flush_dcache_page(page);
> SetPageUptodate(page);
> - unlock_page(page);
> - cifs_readpage_to_fscache(rdata->mapping->host, page);
> }
>
> + unlock_page(page);
> +
> + if (rdata->result == 0)
> + cifs_readpage_to_fscache(rdata->mapping->host, page);
> +
> page_cache_release(page);
> }
> cifs_readdata_free(rdata);
>
> > +
> > + page_cache_release(page);
> > + }
> > + cifs_readdata_free(rdata);
> > +}
> > +
>
Good catch -- patch looks correct to me. Since Steve hasn't merged this
yet, I'll go ahead and roll this into the patch that adds this code.
Thanks,
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
