Re: [PATCH 0/4] ext3 block reservation patch set

[Andrew Morton <akpm@osdl.org>]

Mingming Cao <cmm@us.ibm.com> wrote:
>
> Here is a set of patches which implement the in-memory ext3 block
>  reservation (previously called reservation based ext3 preallocation). 

Great, thanks.  Let's get these in the pipeline.

A few thoughts, from a five-minute read:


- The majority of in-core inodes are not open for reading, and we've
  added 24 bytes to the inode just for inodes which are open for writing.

  At some stage we should stop aggregating struct reserve_window into the
  inode and dynamically allocate it.  We can move i_next_alloc_block,
  i_next_alloc_goal and possibly other fields in there too.

  At which point it has the wrong name ;) Should be `write_state' or
  something.

  It's not clear when we should free up the write_state.  I guess we
  could leave it around for the remaining lifetime of the inode - that'd
  still be a net win.

  Is this something you can look at as a low-priority activity?

- You're performing ext3_discard_reservation() in ext3_release_file(). 
  Note that the file may still have pending allocations at this stage: say,
  open a file, map it MAP_SHARED, dirty some pages which lie over file
  holes then close the file again.

  Later, the VM will come along and write those dirty pages into the
  file, at which point allocations need to be performed.  But we have no
  reservation data and, later, we may have no inode->write_state at all.

  What will happen?

- Have you tested and profiled this with a huge number of open files?  At
  what stage do we get into search complexity problems?

- Why do we discard the file's reservation on every iput()?  iput's are
  relatively common operations. (see fs/fs-writeback.c)

- What locking protects rsv_alloc_hit?  i_sem is not held during
  VM-initiated writeout.  Maybe an atomic_t there, or just say that if we
  race and the number is a bit inaccurate, we don't care?