Hi Vyacheslav,
On Thu, 2 Jan 2014 23:16:22 +0300, Vyacheslav Dubeyko wrote:
> Hi Ryusuke,
>
> On Jan 2, 2014, at 8:17 PM, Ryusuke Konishi wrote:
>
>> Hi, Andreas, and Vyacheslav
>>
>> Thank you for posting this patch.
>>
>> I reviewed the patch, and yes, the patch looks correct and needed to
>> fix the retry logic of nilfs_segctor_collect().
>>
>> I am thinking of sending this to upstream. But, before that, please
>> test the patch enough in the same situation to confirm that it
>> actually fixes the problem and it doesn't cause other issues.
>>
>> At this point, I feel this patch has no defects nor side effects, but
>> I cannot test it in the same situation until the next week.
>>
>
> if (sci->sc_stage.flags & NILFS_CF_SUFREED) {
> err = nilfs_sufile_cancel_freev(nilfs->ns_sufile,
> sci->sc_freesegs,
> sci->sc_nfreesegs,
> NULL);
> WARN_ON(err); /* do not happen */
> + sci->sc_stage.flags &= ~NILFS_CF_SUFREED;
> }
>
> I see adding the code of clear NILFS_CF_SUFREED flag in the patch.
> But nilfs_segctor_abort_construction() contains likewise code:
>
> 1727 if (sci->sc_stage.flags & NILFS_CF_SUFREED) {
> 1728 ret = nilfs_sufile_cancel_freev(nilfs->ns_sufile,
> 1729 sci->sc_freesegs,
> 1730 sci->sc_nfreesegs,
> 1731 NULL);
> 1732 WARN_ON(ret); /* do not happen */
> 1733 }
>
> Maybe, this code needs in flag clearing too?
It is unnecessary for nilfs_segctor_abort_construction() because
nilfs_segctor_abort_construction() is called just before escaping from
nilfs_segctor_do_construct(). The NILFS_CF_SUFREED flag has a meaning
only inside nilfs_segctor_do_construct().
> Anyway, I can see setting this flag and cannot see clearing
> of it. Andreas suggests to clear this flag in the patch.
> Is logic of setting/clearing this flag fully correct?
I think his change is correct, and it is the only call site of
nilfs_sufile_cancel_freev() that needs clearing NILFS_CF_SUFREED flag.
Thank you for your attention to this flag.
Regards,
Ryusuke Konishi
> With the best regards,
> Vyacheslav Dubeyko.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
