From: Sunil Mushran <sunil.mushran@oracle.com> To: Tristan Ye <tristan.ye@oracle.com> Cc: josef@redhat.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-ext4@vger.kernel.org, viro@ZenIV.linux.org.uk, ocfs2-devel@oss.oracle.com Subject: Re: [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() Date: Thu, 19 May 2011 10:45:28 -0700 [thread overview] Message-ID: <4DD55738.3080405@oracle.com> (raw) In-Reply-To: <4DD4DF4C.501@oracle.com> On 05/19/2011 02:13 AM, Tristan Ye wrote: >> + if (inode->i_size == 0 || *offset>= inode->i_size) { >> + ret = -ENXIO; >> + goto out_unlock; >> + } > Why not using if (*offset>= inode->i_size) directly? duh! > + BUG_ON(cpos< le32_to_cpu(rec.e_cpos)); > A same assert has already been performed inside ocfs2_get_clusters_nocache(), > does it make sense to do it again here? good catch >> + >> + if ((!is_data&& origin == SEEK_HOLE) || >> + (is_data&& origin == SEEK_DATA)) { >> + if (extoff> *offset) >> + *offset = extoff; >> + goto out_unlock; > Seems above logic is going to stop at the first time we find a hole. > > How about the offset was within the range of a hole already when we doing > SEEK_HOLE, shouldn't we proceed detecting until the next hole gets found, whose > start_offset was greater than supplied offset, according to semantics described > by the the header of this patch, should it be like following? > > if (extoff> *offset) { > *offset = extoff; > goto out_unlock; > } So if the offset is in a hole, then we set the file pointer to it. Same for data. The file pointer is set to the region asked at an offset that is equal to or greater than the supplied offset. >> + if (origin == SEEK_HOLE) { >> + extoff = cpos; >> + extoff<<= cs_bits; > extoff already has been assigned properly above in while loop? To handle the case when supplied cpos == cend. As always, excellent review. Thanks Sunil
WARNING: multiple messages have this Message-ID (diff)
From: Sunil Mushran <sunil.mushran@oracle.com> To: Tristan Ye <tristan.ye@oracle.com> Cc: josef@redhat.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-ext4@vger.kernel.org, viro@ZenIV.linux.org.uk, ocfs2-devel@oss.oracle.com Subject: [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() Date: Thu, 19 May 2011 10:45:28 -0700 [thread overview] Message-ID: <4DD55738.3080405@oracle.com> (raw) In-Reply-To: <4DD4DF4C.501@oracle.com> On 05/19/2011 02:13 AM, Tristan Ye wrote: >> + if (inode->i_size == 0 || *offset>= inode->i_size) { >> + ret = -ENXIO; >> + goto out_unlock; >> + } > Why not using if (*offset>= inode->i_size) directly? duh! > + BUG_ON(cpos< le32_to_cpu(rec.e_cpos)); > A same assert has already been performed inside ocfs2_get_clusters_nocache(), > does it make sense to do it again here? good catch >> + >> + if ((!is_data&& origin == SEEK_HOLE) || >> + (is_data&& origin == SEEK_DATA)) { >> + if (extoff> *offset) >> + *offset = extoff; >> + goto out_unlock; > Seems above logic is going to stop at the first time we find a hole. > > How about the offset was within the range of a hole already when we doing > SEEK_HOLE, shouldn't we proceed detecting until the next hole gets found, whose > start_offset was greater than supplied offset, according to semantics described > by the the header of this patch, should it be like following? > > if (extoff> *offset) { > *offset = extoff; > goto out_unlock; > } So if the offset is in a hole, then we set the file pointer to it. Same for data. The file pointer is set to the region asked at an offset that is equal to or greater than the supplied offset. >> + if (origin == SEEK_HOLE) { >> + extoff = cpos; >> + extoff<<= cs_bits; > extoff already has been assigned properly above in while loop? To handle the case when supplied cpos == cend. As always, excellent review. Thanks Sunil
next prev parent reply other threads:[~2011-05-19 17:45 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-19 2:44 SEEK_DATA/HOLE on ocfs2 - v2 Sunil Mushran 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran 2011-05-19 2:44 ` [PATCH] fs: Fix default SEEK_DATA and SEEK_HOLE implementation Sunil Mushran 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran 2011-05-19 2:44 ` [PATCH] ocfs2: Implement llseek() Sunil Mushran 2011-05-19 2:44 ` [Ocfs2-devel] " Sunil Mushran 2011-05-19 9:13 ` Tristan Ye 2011-05-19 9:13 ` [Ocfs2-devel] " Tristan Ye 2011-05-19 9:13 ` Tristan Ye 2011-05-19 17:45 ` Sunil Mushran [this message] 2011-05-19 17:45 ` Sunil Mushran 2011-05-19 11:05 ` Christoph Hellwig 2011-05-19 11:05 ` Christoph Hellwig 2011-05-19 17:29 ` Sunil Mushran 2011-05-19 17:29 ` Sunil Mushran 2011-05-19 11:04 ` [Ocfs2-devel] SEEK_DATA/HOLE on ocfs2 - v2 Christoph Hellwig 2011-05-19 11:04 ` Christoph Hellwig 2011-05-23 23:55 PATCH v3: Add support SEEK_DATA/HOLE in ocfs2 Sunil Mushran 2011-05-23 23:55 ` [Ocfs2-devel] [PATCH] ocfs2: Implement llseek() Sunil Mushran
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4DD55738.3080405@oracle.com \ --to=sunil.mushran@oracle.com \ --cc=josef@redhat.com \ --cc=linux-btrfs@vger.kernel.org \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=ocfs2-devel@oss.oracle.com \ --cc=tristan.ye@oracle.com \ --cc=viro@ZenIV.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.