On Thu, Jul 25, 2013 at 07:56:37PM +0800, Zheng Liu wrote: > From: Zheng Liu <wenqing.lz@xxxxxxxxxx> > > ext4_es_remove_extent returns ENOMEM only if we need to split an entry > and insert a part into es tree. After applied this commit (e15f742c), > we have retried to do this. So we don't need to do this again in > ext4_ext_truncate(). > > Signed-off-by: Zheng Liu <wenqing.lz@xxxxxxxxxx> > Cc: "Theodore Ts'o" <tytso@xxxxxxx> Actually, we still need to do this, since the retry loop in __es_remove_extent() tries to shrink the extent status tree for the inode in question, and only retries if we were able to free up some memory. (We only do it for the inode we're working on, since we have it locked already.) So __es_remove_extent() can still return ENOMEM, and so callers of ext4_es_insert_extent() and ext4_es_remove_extent() still need to check for ENOMEM and try to do something sane if possible. The problem with truncate is that the VFS assumes truncate() will always succeed (the method function is returns a void, so there isn't even a way to propagate an error code back p to the VFS), so we really do need to do a retry in ext4's truncate code. For other code paths, like for example fallocate(), it's completely fair game for it to return ENOMEM, although we need to make sure that we've gotten the error handling correct. For the writeback paths, where the application which performed the write may have exited already and we have dirty pages in the page cache, retrying an ENOMEM after calling congestion_wait() is something that *does* make sense. This is why I didn't add an unconditional retry loop to the low-level extent_status tree code, since where we can return ENOMEM, it's better to do that, since that way applications can start failing fast in OOM conditions. Whether or not we want do that is going to depend on the higher level code paths. - Ted -- 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
