Re: [PATCH v2 0/3] xfs: add O_TMPFILE support

Re: [PATCH v2 0/3] xfs: add O_TMPFILE support

[Christoph Hellwig]

Both patches looks good to me,

Reviewed-by: Christoph Hellwig <hch@lst.de>

But now we need to make sure we can exercise it, which will need
support for tmpfile and flink commands in xfs_io.

And tests that use those commands in xfstests.  At a minimum I'd
suggest:

 - test creating read-only/read-write/executable files
 - check how permission bits actually work for O_TMPFILE and
   enforce that (unfortunately I can't find any documentation for that)
 - check that creating non-regular files is properly rejected
 - check that flink works on O_TMPFILE files, but doesn't work on
   on other files

For an additional XFS-specific test I'd recommend:

 - creating a tmpfile, use src/godown to force the filesystem down and
   check that repair recoveres the unlinked inode list
 - creating a tmpfile, flinking it, use src/godown to force the
   filesystem down and check that repair does not have to recover
   the unlink inode list.

On Wed, Dec 18, 2013 at 08:22:38AM +0800, Zhi Yong Wu wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> HI, folks
> 
>   It's time to post the latest revision out, welcome to any constructive comment, thanks.
> 
>   If anyone is interested in playing with it, you can get this patchset from my dev git on github:
>   git://github.com/wuzhy/kernel.git xfs_tmpfile
> 
>   The patchset was tested against the code snippet from Andy Lutomirski and other test cases:
>   http://lwn.net/Articles/562296/
>   If you have any other better test cases, please let me know, thanks.
> 
> #include <stdio.h>
> #include <err.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <string.h>
> 
> #define __O_TMPFILE 020000000
> #define O_DIRECTORY 0200000
> #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
> #define AT_EMPTY_PATH 0x1000
> 
> int main(int argc, char **argv)
> {
>    char buf[128];
> 
>    if (argc != 3)
>      errx(1, "Usage: flinktest PATH linkat|proc");
> 
>    int fd = open(".", O_TMPFILE | O_RDWR, 0600);
>    if (fd == -1)
>      err(1, "O_TMPFILE");
>    else
>      printf("fd #: %d\n", fd);
> 
>    write(fd, "test", 4);
> 
>    if (!strcmp(argv[2], "linkat")) {
>      if (linkat(fd, "", AT_FDCWD, argv[1], AT_EMPTY_PATH) != 0)
>        err(1, "linkat");
>    } else if (!strcmp(argv[2], "proc")) {
>      sprintf(buf, "/proc/self/fd/%d", fd);
>      if (linkat(AT_FDCWD, buf, AT_FDCWD, argv[1], AT_SYMLINK_FOLLOW) != 0)
>        err(1, "linkat");
>    } else {
>      errx(1, "invalid mode");
>    }
> 
>    return 0;
> }
> 
> Changelog from v1:
>  - Fixed one chunk of the comments from Christoph Hellwig and Jeff Liu.
> 
> v1:
>  - Addressed the comments from Dave Chinner and Christoph Hellwig.
> 
> Zhi Yong Wu (3):
>   xfs: factor prid related codes into xfs_get_initial_prid()
>   xfs: add O_TMPFILE support
>   xfs: allow linkat() on O_TMPFILE files
> 
>  fs/xfs/xfs_inode.c      |  123 ++++++++++++++++++++++++++++++++++++++++++++--
>  fs/xfs/xfs_inode.h      |   12 +++++
>  fs/xfs/xfs_iops.c       |   16 ++++++
>  fs/xfs/xfs_shared.h     |    4 +-
>  fs/xfs/xfs_symlink.c    |    5 +--
>  fs/xfs/xfs_trans_resv.c |   55 +++++++++++++++++++--
>  fs/xfs/xfs_trans_resv.h |    2 +
>  7 files changed, 202 insertions(+), 15 deletions(-)
> 
> -- 
> 1.7.6.5
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 0/3] xfs: add O_TMPFILE support

[Zhi Yong Wu]

On Thu, Dec 26, 2013 at 4:51 PM, Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> Both patches looks good to me,
>
> Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Indeed thanks for your review. ok, i will place them to my TODO list.

>
> But now we need to make sure we can exercise it, which will need
> support for tmpfile and flink commands in xfs_io.
>
> And tests that use those commands in xfstests.  At a minimum I'd
> suggest:
>
>  - test creating read-only/read-write/executable files
>  - check how permission bits actually work for O_TMPFILE and
>    enforce that (unfortunately I can't find any documentation for that)
>  - check that creating non-regular files is properly rejected
>  - check that flink works on O_TMPFILE files, but doesn't work on
>    on other files
>
> For an additional XFS-specific test I'd recommend:
>
>  - creating a tmpfile, use src/godown to force the filesystem down and
>    check that repair recoveres the unlinked inode list
>  - creating a tmpfile, flinking it, use src/godown to force the
>    filesystem down and check that repair does not have to recover
>    the unlink inode list.
>
> On Wed, Dec 18, 2013 at 08:22:38AM +0800, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>
>> HI, folks
>>
>>   It's time to post the latest revision out, welcome to any constructive comment, thanks.
>>
>>   If anyone is interested in playing with it, you can get this patchset from my dev git on github:
>>   git://github.com/wuzhy/kernel.git xfs_tmpfile
>>
>>   The patchset was tested against the code snippet from Andy Lutomirski and other test cases:
>>   http://lwn.net/Articles/562296/
>>   If you have any other better test cases, please let me know, thanks.
>>
>> #include <stdio.h>
>> #include <err.h>
>> #include <fcntl.h>
>> #include <unistd.h>
>> #include <string.h>
>>
>> #define __O_TMPFILE 020000000
>> #define O_DIRECTORY 0200000
>> #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
>> #define AT_EMPTY_PATH 0x1000
>>
>> int main(int argc, char **argv)
>> {
>>    char buf[128];
>>
>>    if (argc != 3)
>>      errx(1, "Usage: flinktest PATH linkat|proc");
>>
>>    int fd = open(".", O_TMPFILE | O_RDWR, 0600);
>>    if (fd == -1)
>>      err(1, "O_TMPFILE");
>>    else
>>      printf("fd #: %d\n", fd);
>>
>>    write(fd, "test", 4);
>>
>>    if (!strcmp(argv[2], "linkat")) {
>>      if (linkat(fd, "", AT_FDCWD, argv[1], AT_EMPTY_PATH) != 0)
>>        err(1, "linkat");
>>    } else if (!strcmp(argv[2], "proc")) {
>>      sprintf(buf, "/proc/self/fd/%d", fd);
>>      if (linkat(AT_FDCWD, buf, AT_FDCWD, argv[1], AT_SYMLINK_FOLLOW) != 0)
>>        err(1, "linkat");
>>    } else {
>>      errx(1, "invalid mode");
>>    }
>>
>>    return 0;
>> }
>>
>> Changelog from v1:
>>  - Fixed one chunk of the comments from Christoph Hellwig and Jeff Liu.
>>
>> v1:
>>  - Addressed the comments from Dave Chinner and Christoph Hellwig.
>>
>> Zhi Yong Wu (3):
>>   xfs: factor prid related codes into xfs_get_initial_prid()
>>   xfs: add O_TMPFILE support
>>   xfs: allow linkat() on O_TMPFILE files
>>
>>  fs/xfs/xfs_inode.c      |  123 ++++++++++++++++++++++++++++++++++++++++++++--
>>  fs/xfs/xfs_inode.h      |   12 +++++
>>  fs/xfs/xfs_iops.c       |   16 ++++++
>>  fs/xfs/xfs_shared.h     |    4 +-
>>  fs/xfs/xfs_symlink.c    |    5 +--
>>  fs/xfs/xfs_trans_resv.c |   55 +++++++++++++++++++--
>>  fs/xfs/xfs_trans_resv.h |    2 +
>>  7 files changed, 202 insertions(+), 15 deletions(-)
>>
>> --
>> 1.7.6.5
>>
>> _______________________________________________
>> xfs mailing list
>> xfs-VZNHf3L845pBDgjK7y7TUQ@public.gmane.org
>> http://oss.sgi.com/mailman/listinfo/xfs
> ---end quoted text---



-- 
Regards,

Zhi Yong Wu

Re: [PATCH v2 0/3] xfs: add O_TMPFILE support

[Ben Myers]

Hey Christoph,

On Thu, Dec 26, 2013 at 12:51:34AM -0800, Christoph Hellwig wrote:
> Both patches looks good to me,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But now we need to make sure we can exercise it, which will need
> support for tmpfile and flink commands in xfs_io.

Are you suggesting that we hold off on this until such time as the tests are
completed?
 
Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 0/3] xfs: add O_TMPFILE support

[Christoph Hellwig]

On Tue, Jan 07, 2014 at 02:49:38PM -0600, Ben Myers wrote:
> Are you suggesting that we hold off on this until such time as the tests are
> completed?

Yes.  Code that doesn't come with proper test coverage usually tends to
be buggy.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs