| Message ID | 20200618074950.136553-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: allow btrfs_truncate_block() to fallback to nocow for data space reservation | expand |
On 18/06/2020 09:50, Qu Wenruo wrote: > These two functions have extra conditions that their callers need to > meet, and some not-that-common parameters used for return value. > > So adding some comments may save reviewers some time. These comments have a style/formatting similar to kerneldoc, can you make them real kerneldoc? > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/file.c | 19 +++++++++++++++++++ > fs/btrfs/inode.c | 19 +++++++++++++++++-- > 2 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index fccf5862cd3e..0e4f57fb2737 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -1533,6 +1533,25 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages, > return ret; > } > > +/* > + * Check if we can do nocow write into the range [@pos, @pos + @write_bytes) > + * > + * This function will flush ordered extents in the range to ensure proper > + * nocow checks for (nowait == false) case. > + * > + * Return >0 and update @write_bytes if we can do nocow write into the range. > + * Return 0 if we can't do nocow write. > + * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case, > + * there are ordered extents need to be flushed. > + * Return <0 for if other error happened. > + * > + * NOTE: For wait (nowait==false) calls, callers need to release the drew write > + * lock of inode->root->snapshot_lock if return value > 0. > + * > + * @pos: File offset of the range > + * @write_bytes: The length of the range to check, also contains the nocow > + * writable length if we can do nocow write > + */ > static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos, > size_t *write_bytes, bool nowait) > { > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 86f7aa377da9..48e16eae7278 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -6922,8 +6922,23 @@ static struct extent_map *btrfs_new_extent_direct(struct inode *inode, > } > > /* > - * returns 1 when the nocow is safe, < 1 on error, 0 if the > - * block must be cow'd > + * Check if we can write into [@offset, @offset + @len) of @inode. > + * > + * Return >0 and update @len if we can do nocow write into [@offset, @offset + > + * @len). > + * Return 0 if we can't do nocow write. > + * Return <0 if error happened. > + * > + * NOTE: This only checks the file extents, caller is responsible to wait for > + * any ordered extents. > + * > + * @offset: File offset > + * @len: The length to write, will be updated to the nocow writable > + * range > + * > + * @orig_start: (Optional) Return the original file offset of the file extent > + * @orig_len: (Optional) Return the original on-disk length of the file extent > + * @ram_bytes: (Optional) Return the ram_bytes of the file extent > */ > noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len, > u64 *orig_start, u64 *orig_block_len, >
On 2020/6/18 下午7:57, Johannes Thumshirn wrote: > On 18/06/2020 09:50, Qu Wenruo wrote: >> These two functions have extra conditions that their callers need to >> meet, and some not-that-common parameters used for return value. >> >> So adding some comments may save reviewers some time. > > These comments have a style/formatting similar to kerneldoc, can you make > them real kerneldoc? Mind to give an example? It must be a coincident to match some existing format. Thanks, Qu > >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/file.c | 19 +++++++++++++++++++ >> fs/btrfs/inode.c | 19 +++++++++++++++++-- >> 2 files changed, 36 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c >> index fccf5862cd3e..0e4f57fb2737 100644 >> --- a/fs/btrfs/file.c >> +++ b/fs/btrfs/file.c >> @@ -1533,6 +1533,25 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages, >> return ret; >> } >> >> +/* >> + * Check if we can do nocow write into the range [@pos, @pos + @write_bytes) >> + * >> + * This function will flush ordered extents in the range to ensure proper >> + * nocow checks for (nowait == false) case. >> + * >> + * Return >0 and update @write_bytes if we can do nocow write into the range. >> + * Return 0 if we can't do nocow write. >> + * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case, >> + * there are ordered extents need to be flushed. >> + * Return <0 for if other error happened. >> + * >> + * NOTE: For wait (nowait==false) calls, callers need to release the drew write >> + * lock of inode->root->snapshot_lock if return value > 0. >> + * >> + * @pos: File offset of the range >> + * @write_bytes: The length of the range to check, also contains the nocow >> + * writable length if we can do nocow write >> + */ >> static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos, >> size_t *write_bytes, bool nowait) >> { >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index 86f7aa377da9..48e16eae7278 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -6922,8 +6922,23 @@ static struct extent_map *btrfs_new_extent_direct(struct inode *inode, >> } >> >> /* >> - * returns 1 when the nocow is safe, < 1 on error, 0 if the >> - * block must be cow'd >> + * Check if we can write into [@offset, @offset + @len) of @inode. >> + * >> + * Return >0 and update @len if we can do nocow write into [@offset, @offset + >> + * @len). >> + * Return 0 if we can't do nocow write. >> + * Return <0 if error happened. >> + * >> + * NOTE: This only checks the file extents, caller is responsible to wait for >> + * any ordered extents. >> + * >> + * @offset: File offset >> + * @len: The length to write, will be updated to the nocow writable >> + * range >> + * >> + * @orig_start: (Optional) Return the original file offset of the file extent >> + * @orig_len: (Optional) Return the original on-disk length of the file extent >> + * @ram_bytes: (Optional) Return the ram_bytes of the file extent >> */ >> noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len, >> u64 *orig_start, u64 *orig_block_len, >> >
On 18/06/2020 14:00, Qu Wenruo wrote: > > > On 2020/6/18 下午7:57, Johannes Thumshirn wrote: >> On 18/06/2020 09:50, Qu Wenruo wrote: >>> These two functions have extra conditions that their callers need to >>> meet, and some not-that-common parameters used for return value. >>> >>> So adding some comments may save reviewers some time. >> >> These comments have a style/formatting similar to kerneldoc, can you make >> them real kerneldoc? > > Mind to give an example? It must be a coincident to match some existing > format. > Sure: /** * check_can_nocow() - Check if we can write into [@offset, @offset + @len) of @inode. * * @offset: File offset * @len: The length to write, will be updated to the nocow writable * range * @orig_start: Return the original file offset of the file extent (Optional) * @orig_len: Return the original on-disk length of the file extent (Optional) * @ram_bytes: Return the ram_bytes of the file extent (Optional) * * Return: >0 and update @len if we can do nocow write into [@offset, @offset + @len), * 0 if we can't do nocow write or <0 if error happened. * * NOTE: This only checks the file extents, caller is responsible to wait for * any ordered extents. * */ See also Documentation/doc-guide/kernel-doc.rst for a detailed description Thanks, Johannes
On Thu, Jun 18, 2020 at 12:14:00PM +0000, Johannes Thumshirn wrote: > On 18/06/2020 14:00, Qu Wenruo wrote: > > > > > > On 2020/6/18 下午7:57, Johannes Thumshirn wrote: > >> On 18/06/2020 09:50, Qu Wenruo wrote: > >>> These two functions have extra conditions that their callers need to > >>> meet, and some not-that-common parameters used for return value. > >>> > >>> So adding some comments may save reviewers some time. > >> > >> These comments have a style/formatting similar to kerneldoc, can you make > >> them real kerneldoc? > > > > Mind to give an example? It must be a coincident to match some existing > > format. > > > > Sure: > > /** > * check_can_nocow() - Check if we can write into [@offset, @offset + @len) of @inode. > * > * @offset: File offset > * @len: The length to write, will be updated to the nocow writable > * range > * @orig_start: Return the original file offset of the file extent (Optional) > * @orig_len: Return the original on-disk length of the file extent (Optional) > * @ram_bytes: Return the ram_bytes of the file extent (Optional) > * > * Return: >0 and update @len if we can do nocow write into [@offset, @offset + @len), > * 0 if we can't do nocow write or <0 if error happened. > * > * NOTE: This only checks the file extents, caller is responsible to wait for > * any ordered extents. > * > */ > > See also Documentation/doc-guide/kernel-doc.rst for a detailed description The kernel doc is relatively good, but for our internal helpers I see much point using it and keep the function comment style more relaxed. The short summary, argument description, long description and return values. No strict formatting of the parameters but the "@name: text" looks ok.
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index fccf5862cd3e..0e4f57fb2737 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1533,6 +1533,25 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages, return ret; } +/* + * Check if we can do nocow write into the range [@pos, @pos + @write_bytes) + * + * This function will flush ordered extents in the range to ensure proper + * nocow checks for (nowait == false) case. + * + * Return >0 and update @write_bytes if we can do nocow write into the range. + * Return 0 if we can't do nocow write. + * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case, + * there are ordered extents need to be flushed. + * Return <0 for if other error happened. + * + * NOTE: For wait (nowait==false) calls, callers need to release the drew write + * lock of inode->root->snapshot_lock if return value > 0. + * + * @pos: File offset of the range + * @write_bytes: The length of the range to check, also contains the nocow + * writable length if we can do nocow write + */ static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos, size_t *write_bytes, bool nowait) { diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 86f7aa377da9..48e16eae7278 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -6922,8 +6922,23 @@ static struct extent_map *btrfs_new_extent_direct(struct inode *inode, } /* - * returns 1 when the nocow is safe, < 1 on error, 0 if the - * block must be cow'd + * Check if we can write into [@offset, @offset + @len) of @inode. + * + * Return >0 and update @len if we can do nocow write into [@offset, @offset + + * @len). + * Return 0 if we can't do nocow write. + * Return <0 if error happened. + * + * NOTE: This only checks the file extents, caller is responsible to wait for + * any ordered extents. + * + * @offset: File offset + * @len: The length to write, will be updated to the nocow writable + * range + * + * @orig_start: (Optional) Return the original file offset of the file extent + * @orig_len: (Optional) Return the original on-disk length of the file extent + * @ram_bytes: (Optional) Return the ram_bytes of the file extent */ noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len, u64 *orig_start, u64 *orig_block_len,
These two functions have extra conditions that their callers need to meet, and some not-that-common parameters used for return value. So adding some comments may save reviewers some time. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/file.c | 19 +++++++++++++++++++ fs/btrfs/inode.c | 19 +++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-)