Re: [PATCH v8 03/10] iomap: Complete partial direct I/O writes synchronously

From: Andreas Gruenbacher <agruenba@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: David Sterba <dsterba@suse.cz>,
	cluster-devel <cluster-devel@redhat.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v8 03/10] iomap: Complete partial direct I/O writes synchronously
Date: Wed, 6 Jun 2018 13:44:18 +0200	[thread overview]
Message-ID: <CAHc6FU4pYPy_KQb=zrKX9BxsMBbGw8atC63sOza4Xkvcu-ZcqQ@mail.gmail.com> (raw)
In-Reply-To: <20180606102613.GA11138@lst.de>

On 6 June 2018 at 12:26, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, Jun 05, 2018 at 02:10:24PM +0200, David Sterba wrote:
>> On Mon, Jun 04, 2018 at 09:31:16PM +0200, Andreas Gruenbacher wrote:
>> > @@ -1062,8 +1063,9 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>> >     if (ret < 0)
>> >             iomap_dio_set_error(dio, ret);
>> >
>> > +   smp_mb__before_atomic();
>> >     if (!atomic_dec_and_test(&dio->ref)) {
>>
>> The barrier should be documented. I tried to do a quick look around the
>> code if it's clear why it's there but it's not. Thanks.
>
> It doesn't really make sense.  smp_mb__before_atomic is for relaxed
> atomic operations, which atomic_dec_and_test is not.

After re-reading Documentation/core-api/atomic_ops.rst again, I agree
it's not needed. There is an example in the documentation that was
confusing me.

Thanks,
Andreas