Re: [PATCH 08/19] ceph: address space operations

[Andi Kleen <andi@firstfloor.org>]

Sage Weil <sage@newdream.net> writes:

> The ceph address space methods are concerned primarily with managing
> the dirty page accounting in the inode, which (among other things)
> must keep track of which snapshot context each page was dirtied in,
> and ensure that dirty data is written out to the OSDs in snapshort
> order.
>
> A writepage() on a page that is not currently writeable due to
> snapshot writeback ordering constraints is ignored (it was presumably
> called from kswapd).

Not a detailed review. You would need to get one from someone who
knows the VFS interfaces very well (unfortunately those people are hard
to find). I just read through it.

One thing I noticed is that you seem to do a lot of memory allocation
in the write out paths (some of it even GFP_KERNEL, not GFP_NOFS) 

The traditional wisdom is that you should not allocate memory in block
writeout, because that can deadlock. The worst case is swapfile
on it, but it can happen with mmap too (e.g. one process using
most memory with a file mmap from your fs)  GFP_KERNEL can also recurse,
which can cause other problems in your fs.

There were some changes to make this problem less severe (e.g. better
dirty pages accounting), but I don't think anyone has really declared
it solved yet. The standard workaround for this is to use mempools 
for anything allocated in the writeout path, then you are at least
guaranteed to make forward progress.

You also had at least one unchecked kmalloc I think.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.