On Wed, Aug 20, 2008 at 02:35:35PM -0700, Mingming Cao wrote:
>
> 在 2008-08-20三的 23:57 +0530，Aneesh Kumar K.V写道：
> > On Wed, Aug 20, 2008 at 07:53:31AM -0400, Theodore Tso wrote:
> > > On Wed, Aug 20, 2008 at 04:16:44PM +0530, Aneesh Kumar K.V wrote:
> > > > > mpage_da_map_blocks block allocation failed for inode 323784 at logical
> > > > > offset 313 with max blocks 11 with error -28
> > > > > This should not happen.!! Data will be lost
> > >
> > > We don't actually lose the data if free blocks are subsequently made
> > > available, correct?
> > >
> > > > I tried this patch. There are still multiple ways we can get wrong free
> > > > block count. The patch reduced the number of errors. So we are doing
> > > > better with patch. But I guess we can't use the percpu_counter based
> > > > free block accounting with delalloc. Without delalloc it is ok even if
> > > > we find some wrong free blocks count . The actual block allocation will fail in
> > > > that case and we handle it perfectly fine. With delalloc we cannot
> > > > afford to fail the block allocation. Should we look at a free block
> > > > accounting rewrite using simple ext4_fsblk_t and and a spin lock ?
> > >
> > > It would be a shame if we did given that the whole point of the percpu
> > > counter was to avoid a scalability bottleneck. Perhaps we could take
> > > a filesystem-level spinlock only when the number of free blocks as
> > > reported by the percpu_counter falls below some critical level?
> > >
> > > > --- a/fs/ext4/inode.c
> > > > +++ b/fs/ext4/inode.c
> > > > @@ -1543,7 +1543,14 @@ static int ext4_da_reserve_space(struct inode *inode, int nrblocks)
> > > > }
> > > > /* reduce fs free blocks counter */
> > > > percpu_counter_sub(&sbi->s_freeblocks_counter, total);
> > > > -
> > > > + /*
> > > > + * Now check whether the block count has gone negative.
> > > > + * Some other CPU could have reserved blocks in between
> > > > + */
> > > > + if (percpu_counter_read(&sbi->s_freeblocks_counter) < 0) {
> > > > + spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
> > > > + return -ENOSPC;
> > > > + }
> > >
> > >
> > > I think you want to do the check before calling percpu_counter_sub();
> > > otherwise when you return ENOSPC the free blocks counter ends up
> > > getting reduced (and gets left negative).
> > >
> > > Also, this is one of the places where it might help if we did
> > > something like:
> > >
> > > freeblocks = percpu_counter_read(&sbi->s_freeblocks_counter);
> > > if (freeblocks < NR_CPUS*4)
> > > freeblocks = percpu_counter_sum(&sbi->s_freeblocks_counter);
> > >
> > > if (freeblocks < total) {
> > > spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
> > > return -ENOSPC;
> > > }
> > >
> > > BTW, I was looking at the percpu_counter interface, and I'm confused
> > > why we have percpu_counter_sum_and_set() and percpu_counter_sum(). If
> > > we're taking the fbc->lock to calculate the precise value of the
> > > counter, why not simply set fbc->count?
> > >
> > > Also, it is singularly unfortunate that certain interfaces, such as
> > > percpu_counter_sum_and_set() only exist for CONFIG_SMP. This is
> > > definitely post-2.6.27, but it seems to me that we probably want
> > > something like percpu_counter_compare_lt() which does something like this:
> > >
> > > static inline int percpu_counter_compare_lt(struct percpu_counter *fbc,
> > > s64 amount)
> > > {
> > > #ifdef CONFIG_SMP
> > > if ((fbc->count - amount) < FBC_BATCH)
> > > percpu_counter_sum_and_set(fbc);
> > > #endif
> > > return (fbc->count < amount);
> > > }
> > >
> > > ... which we would then use in ext4_has_free_blocks() and
> > > ext4_da_reserve_space().
> > >
> >
> > Let's say FBC_BATCH = 64 and fbc->count = 100 and we have four cpus and
> > each cpu request for 30 blocks. each CPU does
> >
>
> But, ext4_da_reserve_space() is called at the prepare_write/write_begin
> time for each page to write, so at most per cpu would request 1 block at
> a time, it is not possible to request reserve 30 blocks at a time.
>
> > in ext4_has_free_blocks:
> > free_blocks - nblocks = 100 - 30 = 70 and is > FBC_BATCH So we don't do
>
> free_blocks is not necessary 100,
>
> free_blocks is percpu_counter_read_positive(), which reads the local cpu
> counter. In your example, if the global counter is 100, but the local
> cpu counter is 0, then you will get free_blocks = 0 here. nblocks = 1,
> then you will get
>
> free_blocks - nblocks = 0-1 =-1, which will call
> percpu_counter_sum_and_set() to get more accurate value.
>
> > percpu_counter_sum_and_set
> > That means ext4_has_free_blocks return success
> >
> > Now while claiming blocks we do
> > __percpu_counter_add(fbc, 30, 64)
> >
> > here 30 < 64. That means we don't do fbc->count += count.
> > so fbc->count remains as 100 and we have 4 cpu successfully
> > allocating 30 blocks which means we have to satisfy 120 blocks.
> >
>
> The situation you described here could happen, but really rare and
> should happen at the case fs is really full. The total number of global
> free blocks have to be less than total number of CPU, and there are
> multiple threads write/allocate on each cpu.
>
Current code also get it wrong with a parallel directIO and fallocate.
-aneesh
--
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
