On Wed 18-05-11 14:12:06, Christoph Hellwig wrote:
> >
> > if (unlikely(ret < 0))
> > unlock_page(page);
> > + else {
> > + /*
> > + * Freezing in progress? We check after the page is marked
> > + * dirty and with page lock held so if the test here fails, we
> > + * are sure freezing code will wait during syncing until the
> > + * page fault is done - at that point page will be dirty and
> > + * unlocked so freezing code will write it and writeprotect it
> > + * again.
> > + */
> > + set_page_dirty(page);
> > + if (inode->i_sb->s_frozen != SB_UNFROZEN) {
> > + unlock_page(page);
> > + ret = -EAGAIN;
> > + goto out;
> > + }
> > + }
> > out:
> > return ret;
>
> The code structure looks a bit odd, why not:
>
> if (ret < 0)
> goto out_unlock;
>
> set_page_dirty(page);
> if (inode->i_sb->s_frozen != SB_UNFROZEN) {
> ret = -EAGAIN;
> goto out_unlock;
> }
>
> return 0;
>
> out_unlock:
> unlock_page(page);
> return ret;
> }
>
> Otherwise looks good,
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Thanks, I've changed the flow as you suggested.
Honza
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
