Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Apr 2023 13:26:38 +0530
Message-Id: <877cuezykp.fsf@doe.com>
From:   Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To:     Christoph Hellwig <hch@infradead.org>
Cc:     linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
        Jan Kara <jack@suse.cz>, Christoph Hellwig <hch@infradead.org>,
        "Darrick J . Wong" <djwong@kernel.org>,
        Ojaswin Mujoo <ojaswin@linux.ibm.com>,
        Disha Goel <disgoel@linux.ibm.com>
Subject: Re: [RFCv3 10/10] iomap: Add trace points for DIO path
In-Reply-To: <ZDjs+/T/mf1nHUHI@infradead.org>
Precedence: bulk

Christoph Hellwig <hch@infradead.org> writes:

>> +	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
>>  	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
>>  			     done_before);
>>  	if (IS_ERR_OR_NULL(dio)) {
>> @@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>>  	}
>>  	ret = iomap_dio_complete(dio);
>>  out:
>> +	trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
>
> The trace_iomap_dio_rw_end tracepoint heere seems a bit weird,
> and we'll miss it for file systems using  __iomap_dio_rw directly.

Sorry, yes you are right.

>
> I'd instead add a trace_iomap_dio_rw_queued for the case where
> __iomap_dio_rw returns ERR_PTR(-EIOCBQUEUED), as otherwise we're
> nicely covered by the complete trace points.
>

How about this below change? Does this look good to you?
It should cover all error types and both entry and exit.

And should I fold this entire change in 1 patch or should I split the
refactoring of common out routine into a seperate one?


diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 5871956ee880..e412fdc4ee86 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -130,6 +130,7 @@ ssize_t iomap_dio_complete(struct iomap_dio *dio)
        if (ret > 0)
                ret += dio->done_before;

+       trace_iomap_dio_complete(iocb, dio->error, ret);
        kfree(dio);

        return ret;
@@ -493,12 +494,15 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
        struct blk_plug plug;
        struct iomap_dio *dio;

+       trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
        if (!iomi.len)
-               return NULL;
+               goto out;

        dio = kmalloc(sizeof(*dio), GFP_KERNEL);
-       if (!dio)
-               return ERR_PTR(-ENOMEM);
+       if (!dio) {
+               ret = -ENOMEM;
+               goto out;
+       }

        dio->iocb = iocb;
        atomic_set(&dio->ref, 1);
@@ -650,8 +654,11 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
         */
        dio->wait_for_completion = wait_for_completion;
        if (!atomic_dec_and_test(&dio->ref)) {
-               if (!wait_for_completion)
-                       return ERR_PTR(-EIOCBQUEUED);
+               if (!wait_for_completion) {
+                       ret = -EIOCBQUEUED;
+                       goto out;
+               }
+

                for (;;) {
                        set_current_state(TASK_UNINTERRUPTIBLE);
@@ -663,10 +670,13 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
                __set_current_state(TASK_RUNNING);
        }

+       trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
        return dio;

 out_free_dio:
        kfree(dio);
+out:
+       trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
        if (ret)
                return ERR_PTR(ret);
        return NULL;


>> +		  __print_flags(__entry->dio_flags, "|", TRACE_IOMAP_DIO_STRINGS),
>
> Please avoid the overly lone line here.

Somehow my checkpatch never gave a warning about it.
I will check why was that. But yes, I have anyways made the name to
IOMAP_DIO_STRINGS similar to other namings used in fs/iomap/trace.h

-ritesh