On 2014-09-01 20:43, Andreas Rohner wrote:
> Hi Ryusuke,
> On 2014-09-01 19:59, Ryusuke Konishi wrote:
>> On Sun, 31 Aug 2014 17:47:13 +0200, Andreas Rohner wrote:
>>> Under normal circumstances nilfs_sync_fs() writes out the super block,
>>> which causes a flush of the underlying block device. But this depends on
>>> the THE_NILFS_SB_DIRTY flag, which is only set if the pointer to the
>>> last segment crosses a segment boundary. So if only a small amount of
>>> data is written before the call to nilfs_sync_fs(), no flush of the
>>> block device occurs.
>>>
>>> In the above case an additional call to blkdev_issue_flush() is needed.
>>> To prevent unnecessary overhead, the new flag THE_NILFS_FLUSHED is
>>> introduced, which is cleared whenever new logs are written and set
>>> whenever the block device is flushed.
>>>
>>> Signed-off-by: Andreas Rohner <andreas.rohner@xxxxxxx>
>>
>> The patch looks good to me except that I feel the use of atomic
>> test-and-set bitwise operations something unfavorable (though it's
>> logically correct). I will try to send this to upstream as is unless
>> a comment comes to mind.
>
> I originally thought, that it is necessary to do it atomically to avoid
> a race condition, but I am not so sure about that any more. I think the
> only case we have to avoid is, to call set_nilfs_flushed() after
> blkdev_issue_flush(), because this could race with the
> clear_nilfs_flushed() from the segment construction. So this should also
> work:
>
> + if (wait && !err && nilfs_test_opt(nilfs, BARRIER) &&
> + !nilfs_flushed(nilfs)) {
> + set_nilfs_flushed(nilfs);
> + err = blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);
> + if (err != -EIO)
> + err = 0;
> + }
> +
On the other hand, it says in the comments to set_bit(), that it can be
reordered on architectures other than x86. test_and_set_bit() implies a
memory barrier on all architectures. But I don't think the processor
would reorder set_nilfs_flushed() after the external function call to
blkdev_issue_flush(), would it?
/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
* @addr: the address to start counting from
*
* This function is atomic and may not be reordered. See __set_bit()
* if you do not require the atomic guarantees.
*
* Note: there are no guarantees that this function will not be reordered
* on non x86 architectures, so if you are writing portable code,
* make sure not to rely on its reordering guarantees.
*/
br,
Andreas Rohner
--
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
