On Fri, Oct 6, 2017 at 10:01 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Fri, Oct 06, 2017 at 09:58:58PM +0300, Amir Goldstein wrote:
>> On Fri, Oct 6, 2017 at 8:47 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> > If an inode has been copied up metadata only, then we need to query the
>> > number of blocks from lower and fill up the stat->st_blocks.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
>> > ---
>> > fs/overlayfs/inode.c | 18 ++++++++++++++++--
>> > 1 file changed, 16 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> > index e5825b8948e0..18007634ea9a 100644
>> > --- a/fs/overlayfs/inode.c
>> > +++ b/fs/overlayfs/inode.c
>> > @@ -67,6 +67,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>> > const struct cred *old_cred;
>> > bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
>> > int err;
>> > + bool lowerstat_done = false;
>> > + struct kstat lowerstat;
>> >
>> > type = ovl_path_real(dentry, &realpath);
>> > old_cred = ovl_override_creds(dentry->d_sb);
>> > @@ -86,8 +88,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>> > */
>> > if (ovl_same_sb(dentry->d_sb)) {
>> > if (OVL_TYPE_ORIGIN(type)) {
>> > - struct kstat lowerstat;
>> > - u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
>> > + u32 lowermask = STATX_INO | STATX_BLOCKS |
>> > + (!is_dir ? STATX_NLINK : 0);
>> >
>> > ovl_path_lower(dentry, &realpath);
>> > err = vfs_getattr(&realpath, &lowerstat,
>> > @@ -95,6 +97,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>> > if (err)
>> > goto out;
>> >
>> > + lowerstat_done = true;
>> > WARN_ON_ONCE(stat->dev != lowerstat.dev);
>> > /*
>> > * Lower hardlinks may be broken on copy up to different
>> > @@ -140,6 +143,17 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>> > if (!is_dir && ovl_test_flag(OVL_INDEX, d_inode(dentry)))
>> > stat->nlink = dentry->d_inode->i_nlink;
>> >
>> > + if (ovl_test_flag(OVL_METACOPY, d_inode(dentry))) {
>> > + u32 lowermask = STATX_BLOCKS;
>> > +
>> > + if (!lowerstat_done) {
>>
>> I guess lowerstat_done is as result from my comment,
>> but I did not mean I think vfs_getattr() is expensive,
>> I just didn't like the duplication of that code block, but maybe
>> just a matter of personal taste.
>> Can be sorted out later.
>
> Yes, I added this because I thought you wanted to avoid that extra
> vfs_getattr() call. I am open to change it whatever way you like.
> Personally I did not like this extra boolean and I liked unconditional
> vfs_getattr() better. It was not most optimized but was easier to
> understand.
>
Yeh, I liked it without the boolean better as well.
What I meant is once this patch lands:
https://github.com/chandanr/linux/commit/e9024b0a08a1c725daed548f68ff703305c40124
vfs_getattr() will be called for all non-dir with ORIGIN anyway, so
METACOPY will be able to piggy back STATX_BLOCKS flag on the same call.
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
