Re: [Ocfs2-devel] [PATCH] ocfs2: retry on ENOSPC if sufficient space in truncate log

2016-07-06 Thread Eric Ren 
Hi Joseph,

On 07/06/2016 12:21 PM, Joseph Qi wrote:
> NAK, if ocfs2_try_to_free_truncate_log fails, it will lead to double
> ocfs2_inode_unlock and then BUG.

Thanks for pointing out this! Will fix this and resend.

Eric

>
> On 2016/6/22 17:07, Eric Ren wrote:
>> The testcase "mmaptruncate" in ocfs2 test suite always fails with
>> ENOSPC error on small volume (say less than 10G). This testcase
>> creates 2 threads T1/T2 which race to "truncate"/"extend" a same
>> file repeatedly. Specifically, T1 truncates 1/2 size of a small file
>> while T2 extend to 100% size. The main bitmap will quickly run out
>> of space because the "truncate" code prevent truncate log from being
>> flushed by ocfs2_schedule_truncate_log_flush(osb, 1), while truncate
>> log may have cached lots of clusters.
>>
>> So retry to allocate after flushing truncate log when ENOSPC is
>> returned. And we cannot reuse the deleted blocks before the transaction
>> committed. Fortunately, we already have a function to do this -
>> ocfs2_try_to_free_truncate_log(). Just need to remove the "static"
>> modifier and put it into a right place.
>>
>> Signed-off-by: Eric Ren 
>> ---
>>   fs/ocfs2/alloc.c| 37 +
>>   fs/ocfs2/alloc.h|  2 ++
>>   fs/ocfs2/aops.c | 37 -
>>   fs/ocfs2/suballoc.c | 17 -
>>   4 files changed, 55 insertions(+), 38 deletions(-)
>>
>> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
>> index 460c0ce..7dabbc3 100644
>> --- a/fs/ocfs2/alloc.c
>> +++ b/fs/ocfs2/alloc.c
>> @@ -6106,6 +6106,43 @@ void ocfs2_schedule_truncate_log_flush(struct 
>> ocfs2_super *osb,
>>  }
>>   }
>>
>> +/*
>> + * Try to flush truncate logs if we can free enough clusters from it.
>> + * As for return value, "< 0" means error, "0" no space and "1" means
>> + * we have freed enough spaces and let the caller try to allocate again.
>> + */
>> +int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
>> +unsigned int needed)
>> +{
>> +tid_t target;
>> +int ret = 0;
>> +unsigned int truncated_clusters;
>> +
>> +inode_lock(osb->osb_tl_inode);
>> +truncated_clusters = osb->truncated_clusters;
>> +inode_unlock(osb->osb_tl_inode);
>> +
>> +/*
>> + * Check whether we can succeed in allocating if we free
>> + * the truncate log.
>> + */
>> +if (truncated_clusters < needed)
>> +goto out;
>> +
>> +ret = ocfs2_flush_truncate_log(osb);
>> +if (ret) {
>> +mlog_errno(ret);
>> +goto out;
>> +}
>> +
>> +if (jbd2_journal_start_commit(osb->journal->j_journal, )) {
>> +jbd2_log_wait_commit(osb->journal->j_journal, target);
>> +ret = 1;
>> +}
>> +out:
>> +return ret;
>> +}
>> +
>>   static int ocfs2_get_truncate_log_info(struct ocfs2_super *osb,
>> int slot_num,
>> struct inode **tl_inode,
>> diff --git a/fs/ocfs2/alloc.h b/fs/ocfs2/alloc.h
>> index f3dc1b0..4a5152e 100644
>> --- a/fs/ocfs2/alloc.h
>> +++ b/fs/ocfs2/alloc.h
>> @@ -188,6 +188,8 @@ int ocfs2_truncate_log_append(struct ocfs2_super *osb,
>>u64 start_blk,
>>unsigned int num_clusters);
>>   int __ocfs2_flush_truncate_log(struct ocfs2_super *osb);
>> +int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
>> +   unsigned int needed);
>>
>>   /*
>>* Process local structure which describes the block unlinks done
>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
>> index c034edf..1802aef 100644
>> --- a/fs/ocfs2/aops.c
>> +++ b/fs/ocfs2/aops.c
>> @@ -1645,43 +1645,6 @@ static int ocfs2_zero_tail(struct inode *inode, 
>> struct buffer_head *di_bh,
>>  return ret;
>>   }
>>
>> -/*
>> - * Try to flush truncate logs if we can free enough clusters from it.
>> - * As for return value, "< 0" means error, "0" no space and "1" means
>> - * we have freed enough spaces and let the caller try to allocate again.
>> - */
>> -static int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
>> -  unsigned int needed)
>> -{
>> -tid_t target;
>> -int ret = 0;
>> -unsigned int truncated_clusters;
>> -
>> -inode_lock(osb->osb_tl_inode);
>> -truncated_clusters = osb->truncated_clusters;
>> -inode_unlock(osb->osb_tl_inode);
>> -
>> -/*
>> - * Check whether we can succeed in allocating if we free
>> - * the truncate log.
>> - */
>> -if (truncated_clusters < needed)
>> -goto out;
>> -
>> -ret = ocfs2_flush_truncate_log(osb);
>> -if (ret) {
>> -mlog_errno(ret);
>> -goto out;
>> -}
>> -
>> -if (jbd2_journal_start_commit(osb->journal->j_journal, )) {
>> -jbd2_log_wait_commit(osb->journal->j_journal, target);
>> -ret = 1;
>> -}
>> -out:
>> -

Re: [Ocfs2-devel] [PATCH] ocfs2: retry on ENOSPC if sufficient space in truncate log

2016-07-05 Thread Joseph Qi 
NAK, if ocfs2_try_to_free_truncate_log fails, it will lead to double
ocfs2_inode_unlock and then BUG.

On 2016/6/22 17:07, Eric Ren wrote:
> The testcase "mmaptruncate" in ocfs2 test suite always fails with
> ENOSPC error on small volume (say less than 10G). This testcase
> creates 2 threads T1/T2 which race to "truncate"/"extend" a same
> file repeatedly. Specifically, T1 truncates 1/2 size of a small file
> while T2 extend to 100% size. The main bitmap will quickly run out
> of space because the "truncate" code prevent truncate log from being
> flushed by ocfs2_schedule_truncate_log_flush(osb, 1), while truncate
> log may have cached lots of clusters.
> 
> So retry to allocate after flushing truncate log when ENOSPC is
> returned. And we cannot reuse the deleted blocks before the transaction
> committed. Fortunately, we already have a function to do this -
> ocfs2_try_to_free_truncate_log(). Just need to remove the "static"
> modifier and put it into a right place.
> 
> Signed-off-by: Eric Ren 
> ---
>  fs/ocfs2/alloc.c| 37 +
>  fs/ocfs2/alloc.h|  2 ++
>  fs/ocfs2/aops.c | 37 -
>  fs/ocfs2/suballoc.c | 17 -
>  4 files changed, 55 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index 460c0ce..7dabbc3 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -6106,6 +6106,43 @@ void ocfs2_schedule_truncate_log_flush(struct 
> ocfs2_super *osb,
>   }
>  }
>  
> +/*
> + * Try to flush truncate logs if we can free enough clusters from it.
> + * As for return value, "< 0" means error, "0" no space and "1" means
> + * we have freed enough spaces and let the caller try to allocate again.
> + */
> +int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> + unsigned int needed)
> +{
> + tid_t target;
> + int ret = 0;
> + unsigned int truncated_clusters;
> +
> + inode_lock(osb->osb_tl_inode);
> + truncated_clusters = osb->truncated_clusters;
> + inode_unlock(osb->osb_tl_inode);
> +
> + /*
> +  * Check whether we can succeed in allocating if we free
> +  * the truncate log.
> +  */
> + if (truncated_clusters < needed)
> + goto out;
> +
> + ret = ocfs2_flush_truncate_log(osb);
> + if (ret) {
> + mlog_errno(ret);
> + goto out;
> + }
> +
> + if (jbd2_journal_start_commit(osb->journal->j_journal, )) {
> + jbd2_log_wait_commit(osb->journal->j_journal, target);
> + ret = 1;
> + }
> +out:
> + return ret;
> +}
> +
>  static int ocfs2_get_truncate_log_info(struct ocfs2_super *osb,
>  int slot_num,
>  struct inode **tl_inode,
> diff --git a/fs/ocfs2/alloc.h b/fs/ocfs2/alloc.h
> index f3dc1b0..4a5152e 100644
> --- a/fs/ocfs2/alloc.h
> +++ b/fs/ocfs2/alloc.h
> @@ -188,6 +188,8 @@ int ocfs2_truncate_log_append(struct ocfs2_super *osb,
> u64 start_blk,
> unsigned int num_clusters);
>  int __ocfs2_flush_truncate_log(struct ocfs2_super *osb);
> +int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> +unsigned int needed);
>  
>  /*
>   * Process local structure which describes the block unlinks done
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index c034edf..1802aef 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -1645,43 +1645,6 @@ static int ocfs2_zero_tail(struct inode *inode, struct 
> buffer_head *di_bh,
>   return ret;
>  }
>  
> -/*
> - * Try to flush truncate logs if we can free enough clusters from it.
> - * As for return value, "< 0" means error, "0" no space and "1" means
> - * we have freed enough spaces and let the caller try to allocate again.
> - */
> -static int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> -   unsigned int needed)
> -{
> - tid_t target;
> - int ret = 0;
> - unsigned int truncated_clusters;
> -
> - inode_lock(osb->osb_tl_inode);
> - truncated_clusters = osb->truncated_clusters;
> - inode_unlock(osb->osb_tl_inode);
> -
> - /*
> -  * Check whether we can succeed in allocating if we free
> -  * the truncate log.
> -  */
> - if (truncated_clusters < needed)
> - goto out;
> -
> - ret = ocfs2_flush_truncate_log(osb);
> - if (ret) {
> - mlog_errno(ret);
> - goto out;
> - }
> -
> - if (jbd2_journal_start_commit(osb->journal->j_journal, )) {
> - jbd2_log_wait_commit(osb->journal->j_journal, target);
> - ret = 1;
> - }
> -out:
> - return ret;
> -}
> -
>  int ocfs2_write_begin_nolock(struct address_space *mapping,
>loff_t pos, unsigned len, ocfs2_write_type_t type,
>

Re: [Ocfs2-devel] [PATCH] ocfs2: retry on ENOSPC if sufficient space in truncate log

2016-06-28 Thread Joseph Qi 
Looks good to me, thanks.
Reviewed-by: Joseph Qi 

On 2016/6/22 17:07, Eric Ren wrote:
> The testcase "mmaptruncate" in ocfs2 test suite always fails with
> ENOSPC error on small volume (say less than 10G). This testcase
> creates 2 threads T1/T2 which race to "truncate"/"extend" a same
> file repeatedly. Specifically, T1 truncates 1/2 size of a small file
> while T2 extend to 100% size. The main bitmap will quickly run out
> of space because the "truncate" code prevent truncate log from being
> flushed by ocfs2_schedule_truncate_log_flush(osb, 1), while truncate
> log may have cached lots of clusters.
> 
> So retry to allocate after flushing truncate log when ENOSPC is
> returned. And we cannot reuse the deleted blocks before the transaction
> committed. Fortunately, we already have a function to do this -
> ocfs2_try_to_free_truncate_log(). Just need to remove the "static"
> modifier and put it into a right place.
> 
> Signed-off-by: Eric Ren 
> ---
>  fs/ocfs2/alloc.c| 37 +
>  fs/ocfs2/alloc.h|  2 ++
>  fs/ocfs2/aops.c | 37 -
>  fs/ocfs2/suballoc.c | 17 -
>  4 files changed, 55 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index 460c0ce..7dabbc3 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -6106,6 +6106,43 @@ void ocfs2_schedule_truncate_log_flush(struct 
> ocfs2_super *osb,
>   }
>  }
>  
> +/*
> + * Try to flush truncate logs if we can free enough clusters from it.
> + * As for return value, "< 0" means error, "0" no space and "1" means
> + * we have freed enough spaces and let the caller try to allocate again.
> + */
> +int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> + unsigned int needed)
> +{
> + tid_t target;
> + int ret = 0;
> + unsigned int truncated_clusters;
> +
> + inode_lock(osb->osb_tl_inode);
> + truncated_clusters = osb->truncated_clusters;
> + inode_unlock(osb->osb_tl_inode);
> +
> + /*
> +  * Check whether we can succeed in allocating if we free
> +  * the truncate log.
> +  */
> + if (truncated_clusters < needed)
> + goto out;
> +
> + ret = ocfs2_flush_truncate_log(osb);
> + if (ret) {
> + mlog_errno(ret);
> + goto out;
> + }
> +
> + if (jbd2_journal_start_commit(osb->journal->j_journal, )) {
> + jbd2_log_wait_commit(osb->journal->j_journal, target);
> + ret = 1;
> + }
> +out:
> + return ret;
> +}
> +
>  static int ocfs2_get_truncate_log_info(struct ocfs2_super *osb,
>  int slot_num,
>  struct inode **tl_inode,
> diff --git a/fs/ocfs2/alloc.h b/fs/ocfs2/alloc.h
> index f3dc1b0..4a5152e 100644
> --- a/fs/ocfs2/alloc.h
> +++ b/fs/ocfs2/alloc.h
> @@ -188,6 +188,8 @@ int ocfs2_truncate_log_append(struct ocfs2_super *osb,
> u64 start_blk,
> unsigned int num_clusters);
>  int __ocfs2_flush_truncate_log(struct ocfs2_super *osb);
> +int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> +unsigned int needed);
>  
>  /*
>   * Process local structure which describes the block unlinks done
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index c034edf..1802aef 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -1645,43 +1645,6 @@ static int ocfs2_zero_tail(struct inode *inode, struct 
> buffer_head *di_bh,
>   return ret;
>  }
>  
> -/*
> - * Try to flush truncate logs if we can free enough clusters from it.
> - * As for return value, "< 0" means error, "0" no space and "1" means
> - * we have freed enough spaces and let the caller try to allocate again.
> - */
> -static int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> -   unsigned int needed)
> -{
> - tid_t target;
> - int ret = 0;
> - unsigned int truncated_clusters;
> -
> - inode_lock(osb->osb_tl_inode);
> - truncated_clusters = osb->truncated_clusters;
> - inode_unlock(osb->osb_tl_inode);
> -
> - /*
> -  * Check whether we can succeed in allocating if we free
> -  * the truncate log.
> -  */
> - if (truncated_clusters < needed)
> - goto out;
> -
> - ret = ocfs2_flush_truncate_log(osb);
> - if (ret) {
> - mlog_errno(ret);
> - goto out;
> - }
> -
> - if (jbd2_journal_start_commit(osb->journal->j_journal, )) {
> - jbd2_log_wait_commit(osb->journal->j_journal, target);
> - ret = 1;
> - }
> -out:
> - return ret;
> -}
> -
>  int ocfs2_write_begin_nolock(struct address_space *mapping,
>loff_t pos, unsigned len, ocfs2_write_type_t type,
>struct page **pagep, void **fsdata,

Re: [Ocfs2-devel] [PATCH] ocfs2: retry on ENOSPC if sufficient space in truncate log

2016-06-23 Thread Gang He 
Reviewed-by: Gang He 

Thanks
Gang


>>> 
> The testcase "mmaptruncate" in ocfs2 test suite always fails with
> ENOSPC error on small volume (say less than 10G). This testcase
> creates 2 threads T1/T2 which race to "truncate"/"extend" a same
> file repeatedly. Specifically, T1 truncates 1/2 size of a small file
> while T2 extend to 100% size. The main bitmap will quickly run out
> of space because the "truncate" code prevent truncate log from being
> flushed by ocfs2_schedule_truncate_log_flush(osb, 1), while truncate
> log may have cached lots of clusters.
> 
> So retry to allocate after flushing truncate log when ENOSPC is
> returned. And we cannot reuse the deleted blocks before the transaction
> committed. Fortunately, we already have a function to do this -
> ocfs2_try_to_free_truncate_log(). Just need to remove the "static"
> modifier and put it into a right place.
> 
> Signed-off-by: Eric Ren 
> ---
>  fs/ocfs2/alloc.c| 37 +
>  fs/ocfs2/alloc.h|  2 ++
>  fs/ocfs2/aops.c | 37 -
>  fs/ocfs2/suballoc.c | 17 -
>  4 files changed, 55 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index 460c0ce..7dabbc3 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -6106,6 +6106,43 @@ void ocfs2_schedule_truncate_log_flush(struct 
> ocfs2_super *osb,
>   }
>  }
>  
> +/*
> + * Try to flush truncate logs if we can free enough clusters from it.
> + * As for return value, "< 0" means error, "0" no space and "1" means
> + * we have freed enough spaces and let the caller try to allocate again.
> + */
> +int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> + unsigned int needed)
> +{
> + tid_t target;
> + int ret = 0;
> + unsigned int truncated_clusters;
> +
> + inode_lock(osb->osb_tl_inode);
> + truncated_clusters = osb->truncated_clusters;
> + inode_unlock(osb->osb_tl_inode);
> +
> + /*
> +  * Check whether we can succeed in allocating if we free
> +  * the truncate log.
> +  */
> + if (truncated_clusters < needed)
> + goto out;
> +
> + ret = ocfs2_flush_truncate_log(osb);
> + if (ret) {
> + mlog_errno(ret);
> + goto out;
> + }
> +
> + if (jbd2_journal_start_commit(osb->journal->j_journal, )) {
> + jbd2_log_wait_commit(osb->journal->j_journal, target);
> + ret = 1;
> + }
> +out:
> + return ret;
> +}
> +
>  static int ocfs2_get_truncate_log_info(struct ocfs2_super *osb,
>  int slot_num,
>  struct inode **tl_inode,
> diff --git a/fs/ocfs2/alloc.h b/fs/ocfs2/alloc.h
> index f3dc1b0..4a5152e 100644
> --- a/fs/ocfs2/alloc.h
> +++ b/fs/ocfs2/alloc.h
> @@ -188,6 +188,8 @@ int ocfs2_truncate_log_append(struct ocfs2_super *osb,
> u64 start_blk,
> unsigned int num_clusters);
>  int __ocfs2_flush_truncate_log(struct ocfs2_super *osb);
> +int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> +unsigned int needed);
>  
>  /*
>   * Process local structure which describes the block unlinks done
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index c034edf..1802aef 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -1645,43 +1645,6 @@ static int ocfs2_zero_tail(struct inode *inode, struct 
> buffer_head *di_bh,
>   return ret;
>  }
>  
> -/*
> - * Try to flush truncate logs if we can free enough clusters from it.
> - * As for return value, "< 0" means error, "0" no space and "1" means
> - * we have freed enough spaces and let the caller try to allocate again.
> - */
> -static int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
> -   unsigned int needed)
> -{
> - tid_t target;
> - int ret = 0;
> - unsigned int truncated_clusters;
> -
> - inode_lock(osb->osb_tl_inode);
> - truncated_clusters = osb->truncated_clusters;
> - inode_unlock(osb->osb_tl_inode);
> -
> - /*
> -  * Check whether we can succeed in allocating if we free
> -  * the truncate log.
> -  */
> - if (truncated_clusters < needed)
> - goto out;
> -
> - ret = ocfs2_flush_truncate_log(osb);
> - if (ret) {
> - mlog_errno(ret);
> - goto out;
> - }
> -
> - if (jbd2_journal_start_commit(osb->journal->j_journal, )) {
> - jbd2_log_wait_commit(osb->journal->j_journal, target);
> - ret = 1;
> - }
> -out:
> - return ret;
> -}
> -
>  int ocfs2_write_begin_nolock(struct address_space *mapping,
>loff_t pos, unsigned len, ocfs2_write_type_t type,
>struct page **pagep, void **fsdata,
> diff --git a/fs/ocfs2/suballoc.c